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

Escape boolean alias strings to play nice with yaml 1.1 implementations #86

Closed
wants to merge 2 commits into from

Conversation

kapilt
Copy link

@kapilt kapilt commented Jul 24, 2013

Per issue #85

@ixti
Copy link
Contributor

ixti commented Jul 24, 2013

js-yaml implements YAML 1.2 spec. So this backward compatibility mode with 1.1 should be optional.

@puzrin
Copy link
Member

puzrin commented Jul 24, 2013

@Kapit, your solution ignores schemas/types concept, used in js-yaml. That breaks architecture. I'm not ready to accept such pull request.

@kapilt
Copy link
Author

kapilt commented Jul 24, 2013

understood. i'll have a look through the schema/types parts. re optional
with 1.2, compatibility would be triggered by param to load/dump? afaik
this particular item is the only compatibility issue between 1.1/1.2, are
there others? thanks

On Wed, Jul 24, 2013 at 6:28 PM, Vitaly Puzrin notifications@github.comwrote:

@Kapit https://github.com/kapit, your solution ignores schemas/types
concept, used in js-yaml. That breaks architecture. I'm not ready to accept
such pull request.


Reply to this email directly or view it on GitHubhttps://github.com//pull/86#issuecomment-21521322
.

@puzrin
Copy link
Member

puzrin commented Jul 24, 2013

AFAIK, it's the only major difference.

Dumper already check known implicits, to force quoting for conflicting names https://github.com/nodeca/js-yaml/blob/master/lib/js-yaml/dumper.js#L223 . Probably, we should change false=>true here https://github.com/nodeca/js-yaml/blob/master/lib/js-yaml/dumper.js#L223 to check explicits too. Boolean type already have all necessary definitions.

/cc @dervus

@kapilt
Copy link
Author

kapilt commented Jul 25, 2013

thanks. changing https://github.com/nodeca/js-yaml/blob/master/lib/js-yaml/dumper.js#L155 to true, does indeed resolve. afaics booleans are the only type that even checks the explicit param.

@puzrin
Copy link
Member

puzrin commented Jul 25, 2013

Ok. I close this pull request as invalid, but issue still left open. Use fork please, until we do correct fix in mainstream. That's more complicated, that you can decide at first glance.

@puzrin puzrin closed this Jul 25, 2013
@dervus dervus reopened this Aug 11, 2013
@dervus dervus closed this Aug 13, 2013
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

4 participants