Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: multi paragraphs parsing #15

Merged
merged 3 commits into from
Sep 12, 2021
Merged

fix: multi paragraphs parsing #15

merged 3 commits into from
Sep 12, 2021

Conversation

gr2m
Copy link
Collaborator

@gr2m gr2m commented Sep 12, 2021

fixes #14

@gr2m gr2m force-pushed the multi-paragraphs-parsing branch 2 times, most recently from a19bc3d to 7e9878b Compare September 12, 2021 17:10
Copy link
Owner

@stefanbuck stefanbuck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing, you're so fast fixing things 😄 I added on comment about an addition case for a single line break.

@@ -0,0 +1,3 @@
{
"textarea": "1st paragraph\n\n2nd paragraph"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe worth adding another case for a single line break since it is not covered yet.

1st paragraph\n2nd paragraph

If you don't mind, I'm happy to make the change on top of your changes and push it onto this branch.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here it is 5360a1d.

While updating the id from textarea to textarea-one, I stumbled across the replacing we do and I wonder if this is actually necessary. If I remember correctly, this was done to produce a nice looking output variable, but in the JSON output this is quite confusing. What do you think, should we get rid of this replace?

return idMapping[str].replace(/-/g, "_");

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't mind, I'm happy to make the change on top of your changes and push it onto this branch.

no problem 👍🏼

While updating the id from textarea to textarea-one, I stumbled across the replacing we do and I wonder if this is actually necessary. If I remember correctly, this was done to produce a nice looking output variable, but in the JSON output this is quite confusing. What do you think, should we get rid of this replace?

Yeah +1 on getting rid of that. I think it will confuse people, I don't think this behavior is currently documented either. But maybe fix that in a separate PR?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, I will work on a pull request tomorrow evening. This is a breaking change so I'll update the workflow to push a v2 branch JTLYK.

@gr2m gr2m merged commit e78326b into main Sep 12, 2021
@gr2m gr2m deleted the multi-paragraphs-parsing branch September 12, 2021 20:50
@github-actions
Copy link

🎉 This PR is included in version 1.0.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

stefanbuck pushed a commit that referenced this pull request Sep 13, 2021
BREAKING CHANGE:
Before this change, IDs defined in a GitHub issue template that included a hyphen got converted to dashes for the Action output. This was mainly done to ensure nice-looking output variables. However, this is confusing as discussed in #15 (comment).
gr2m pushed a commit that referenced this pull request Sep 13, 2021
BREAKING CHANGE: Before this change, IDs defined in a GitHub issue template that included a hyphen got converted to dashes for the Action output. This was mainly done to ensure nice-looking output variables. However, this is confusing as discussed in #15 (comment).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Only 1st paragraphed is taken from value
2 participants