Fix condenseFlow for objects

[eseliger]

Closes #370

[puzrin]

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

[eseliger]

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]

@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]

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]

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]

@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]

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]

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]

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]

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

[puzrin]

Published