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 condenseFlow for objects #371

Merged
merged 4 commits into from Sep 11, 2017
Merged

Fix condenseFlow for objects #371

merged 4 commits into from Sep 11, 2017

Conversation

eseliger
Copy link
Contributor

@eseliger eseliger commented Sep 6, 2017

Closes #370

@puzrin
Copy link
Member

puzrin commented Sep 6, 2017

That's not self-sufficient and strange. Like attempt to fix one kludge with another kludge. Why not fix condenseFlow instead?

@eseliger
Copy link
Contributor Author

eseliger commented Sep 6, 2017

I could also just have fixed the option by not doing anything with dumped objects, true.
But, according to the specification, the two valid ways to dump a object are either using the space or quoting the key and (optionally) omitting the space.
So I think it's the best way to introduce this option, because the original functionality of the condenseFlow option was to dump yaml without spaces (except for those in strings), to be used in urls. With the quote keys option this is still possible and we cover both ways yaml allows by spec :)

@felixfbecker
Copy link

@puzrin if you look at the tests this actually does fix condenseFlow. But for the use case of URLs, {'key':value} can be nicer than {key:%20value}

@puzrin
Copy link
Member

puzrin commented Sep 6, 2017

I'm strongly against adding options "just for fun". Dumper should provide correct output. If condenseFlow can not do that, it should be fixed. For example - by adding quotes. But not via adding one more option.

@eseliger
Copy link
Contributor Author

eseliger commented Sep 6, 2017

I don't think it is "just for fun", but another step towards supporting the whole yaml spec with this library. I'm don't think condenseFlow should just "magically" quote keys, but the consumer of the api should be able to specify what fits better for him, especially which quotes are the ones he wants. That would add another option, too, if the current condenseFlow would just add this behaviour.

@felixfbecker
Copy link

felixfbecker commented Sep 6, 2017

@puzrin please read my comment above, this actually fixes condenseFlow even without the quoteKeys option. If you just use condenseFlow: true without quoteKeys, there are now proper spaces, whereas before there were not.

The quoteKeys option is introduced here for the same use case the condenseFlow flag itself was added originally, avoiding unneeded spaces in the output for use in environments where spaces need to be escaped in cumbersome ways (e.g. URLs, terminals, ...).

Hope this makes it more clear.

@puzrin
Copy link
Member

puzrin commented Sep 6, 2017

If i start add everything possible to imagine, api will become messy very quickly. So, i prefer to be gready.

There were no requests for new quoting option. The only consumers are you, with specific use case for condenceFlow (invented by you too). I think, it will be better to introduce condenced format improvement and keep api simple.

If this PR contains fix for contenceFlow - you can split it separate for faster merge.

@felixfbecker
Copy link

There were no requests for new quoting option

I count two requests :)

As an open source maintainer myself I totally understand that API bloat is a thing and you want to keep the API minimal, but I think the option is justified here. Structured URL serialisation formats are becoming more common as applications become more complex, e.g. this is a URL I just copied from Expensify (could be so much prettier in YAML):

https://www.expensify.com/expenses#param={"expenseType":"unsubmitted","viewType":"table","card":-1,"category":-1,"tag":-1,"accountEmail":-1,"policyID":-1,"groupby":"none","graphby":"monthly","startDate":"2017-07-18","endDate":"2017-09-06","merchant":""}

While some users may want a super-minimal YAML parser, I think they are already well served with options like tj/js-yaml (389 lines).

This project on the other hand already has a lot of options for greater flexibility, and the PR only adds a minimal amount of new code lines. And I guess this PR is proof itself that there is continuous interest and willingness to maintain this ;)

I would be happy to get both the fix and the option merged.

@puzrin
Copy link
Member

puzrin commented Sep 6, 2017

I still stay, that:

  • option should be added only when it's impossible without it.
  • option should not be added for fun or in advance, only when real world use case exists.

Right now IMHO it worth to add keys quoting to condenceFlow. Later, if anyone can explain that separate option needed, we can split it. Right now i did not found reasons, why separate option is better than format fix.

@eseliger eseliger changed the title Add quote keys option to fix condenseFlow for objects Fix condenseFlow for objects Sep 6, 2017
@eseliger
Copy link
Contributor Author

eseliger commented Sep 6, 2017

Updated it to just fix the problem as of now, please let me know what else to do to merge quickly :)

@puzrin puzrin merged commit 0ec1037 into nodeca:master Sep 11, 2017
@puzrin
Copy link
Member

puzrin commented Sep 11, 2017

Published

minj added a commit to minj/js-yaml that referenced this pull request Apr 28, 2018
* master: (58 commits)
  Check for leading newlines when determining if block indentation indicator is needed (nodeca#404)
  Add property based tests to assess load reverses dump (nodeca#398)
  3.11.0 released
  Browser files rebuild
  Dumper: fix negative integers in bin/octal/hex formats, close nodeca#399
  support es6 arrow functions, fixes nodeca#389 (nodeca#393)
  Fix typo in README.md (nodeca#373)
  3.10.0 released
  Browser files rebuild
  Add test for astrals dump
  Combine surrogate pairs into one escape sequence when encoding. (nodeca#369)
  Fix condenseFlow for objects (nodeca#371)
  correct spelling mistake (nodeca#367)
  More meaningful error for loader (nodeca#361)
  Fix typo and format code. (nodeca#365)
  3.9.1 released
  Browser files rebuild
  Ensure stack is present for custom errors (fixes nodeca#351) (nodeca#360)
  3.9.0 released
  Browser files rebuild
  ...
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.

None yet

3 participants