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

Add ability to override quoting style for scalar values #454

Closed
wants to merge 1 commit into from

Conversation

jpb
Copy link

@jpb jpb commented Oct 29, 2018

This is a reimplementation of #291 and applies the feedback from the original implementation in #291 (comment), except in two cases:

  • the option name has been changed from valueFlowScalars to scalarQuoteStyle as the original implementation was (and my use-case is) not limited to flow values
  • I chose to not accept STYLE_PLAIN as a value. This didn't make sense to me as I feel this option should never "downgrade" a values quoting – js-yaml only uses quotes when necessary to make valid YAML so removing single or double quotes would to make the values "plain" would produce invalid YAML. This is also why STYLE_PLAIN will have no effect for double quoted values (as the readme states).

Fixes #290

@jpb jpb force-pushed the scalar-quote-style branch 3 times, most recently from 8bebfe7 to 996dc4f Compare October 29, 2018 23:49
@puzrin
Copy link
Member

puzrin commented Oct 30, 2018

Implementation is very dirty:

  • multiple value types on input
  • strings compare in hot path

What should be done:

  • use empty string / ' / " as input.
  • normalize input value (string) to number, before run.
  • something should be done with option name (see comments in prev commit), need to discuss

@yihongang
Copy link

@jpb are you still interested in continuing with this work?

@puzrin could you explain what do you mean by input here? is it the null default value vs the expected string values? with regards to the option name what do you have in mind?

@mansona
Copy link

mansona commented Apr 10, 2019

I have just tested this and it's working really nicely 🎉 seems like something that would be good to look to merge

@nicmosc
Copy link

nicmosc commented Feb 14, 2020

What's the status on this? Really need this feature! 🙏🏻

@kevthehermit
Copy link

Any movement on this?

@SkinnyFatSumo
Copy link

Any chance this feature will come through anytime soon? It'd be very helpful

@rlidwka
Copy link
Member

rlidwka commented Dec 9, 2020

I've added more extensive support for this feature in 7b256d7 (located in dev branch currently).

Closing this as outdated. Thanks for posting this PR, helped to find some corner cases I haven't originally thought of.

@rlidwka rlidwka closed this Dec 9, 2020
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.

[writer]Surround string by double quote instead of simple
8 participants