search

stoplightio / yaml

Useful functions when working with YAML.
Apache License 2.0
13 stars 3 forks source link

fix(safe-stringify): stringify falsy values and strings #37

Closed P0lip closed 4 years ago

P0lip commented 4 years ago

Needed by https://github.com/stoplightio/json-schema-ref-parser/pull/1 (2 tests fail due to this issue) The caveat is... we'd be inconsistent with https://github.com/stoplightio/json/blob/master/src/safeStringify.ts#L10 that implements a similar logic. I know we do it to avoid stringifying twice, so - I'm happy to simply change that test in json-schema-ref-parser. Would still love to keep that removal of falsy check.

marbemac commented 4 years ago

Yeah I don't really like this change - why would we add /n when stringifying? And re falsy check, what do we get by removing it again?

P0lip commented 4 years ago

Yeah I don't really like this change - why would we add /n when stringifying?

This is actually on purposed and this PR doesn't change it - if you take a closer look, the previous expected value did have a new value as well. We just used a template literal that made it more difficult to spot it.

This is what both js-yaml's dump and our fork of yaml-ast-parser (which is also a fork of js-yaml) do https://github.com/nodeca/js-yaml/blob/master/lib/js-yaml/dumper.js#L840 https://github.com/stoplightio/yaml-ast-parser/blob/master/src/dumper.ts#L855

This PR doesn't change anything in regards to that.

And re falsy check, what do we get by removing it again?

We can stringify 0, null, false, etc. Right now we don't do it and this is a bug.

marbemac commented 4 years ago

We can stringify 0, null, false, etc. Right now we don't do it and this is a bug.

Got it - ok cool let's trim this down and keep this bit, but remove the bits that make this inconsistent w json safeStringify.

P0lip commented 4 years ago

@marbemac @pytlesk4 reverted. Ready for another pass.

stoplight-bot commented 4 years ago

:tada: This PR is included in version 3.5.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket: