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

Improve quoted string flexibility #177

Merged
merged 4 commits into from
Jul 8, 2020
Merged

Improve quoted string flexibility #177

merged 4 commits into from
Jul 8, 2020

Conversation

Himura2la
Copy link
Contributor

We use this library to update files, which are also updated by another (YamlDotNet) library, and we have quoting style not in sync, which generates useless changes. I added an option to choose the default quoting style, and also tried to avoid escaped single quotes within string content.

And also use double-quoted strings for strings with "'",
which minimizes escaping
@Himura2la
Copy link
Contributor Author

Himura2la commented Jun 22, 2020

@eemeli, looks like checks failed not because of my changes

Copy link
Owner

@eemeli eemeli left a comment

Choose a reason for hiding this comment

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

Finally got around to this; apologies for the delay. I like the sense of this, but I think I'd prefer a boolean option, given that is has only two valid values.

src/stringify/stringifyString.js Outdated Show resolved Hide resolved
src/stringify/stringifyString.js Outdated Show resolved Hide resolved
src/tags/options.js Outdated Show resolved Hide resolved
@Himura2la
Copy link
Contributor Author

Thanks for your review, I'll change the code according to your comments ASAP

@Himura2la Himura2la requested a review from eemeli July 6, 2020 07:35
index.d.ts Outdated Show resolved Hide resolved
src/stringify/stringifyString.js Outdated Show resolved Hide resolved
Co-authored-by: Eemeli Aro <eemeli@gmail.com>
@Himura2la
Copy link
Contributor Author

Thank you for your suggestions, it's much better now

@Himura2la Himura2la requested a review from eemeli July 8, 2020 07:19
@eemeli eemeli merged commit f118002 into eemeli:master Jul 8, 2020
@eemeli
Copy link
Owner

eemeli commented Jul 8, 2020

Thank you @Himura2la!

@Himura2la
Copy link
Contributor Author

Himura2la commented Jul 9, 2020

@eemeli thanks for merging this helpful option. When do you plan to make it available in npm?

@eemeli
Copy link
Owner

eemeli commented Jul 22, 2020

Sorry, missed your last comment. I'll try and get a 2.0.0-alpha.1 release out within the next week or so.

@Himura2la
Copy link
Contributor Author

@eemeli, just a friendly bump: we can't close a ticket in our system unless this PR is released.

@eemeli
Copy link
Owner

eemeli commented Aug 23, 2020

Sorry it took so long, but 2.0.0-0 is now finally out, including this fix. Release notes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants