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

Missing escaping for uppercase "No" #188

Closed
Zinggi opened this issue Mar 12, 2024 · 4 comments · Fixed by #190
Closed

Missing escaping for uppercase "No" #188

Zinggi opened this issue Mar 12, 2024 · 4 comments · Fixed by #190

Comments

@Zinggi
Copy link
Contributor

Zinggi commented Mar 12, 2024

Issue description

Similar to #184, I've found another issue with escaping:

iex> Ymlr.document!(%{foo: "No"})
"---\nfoo: No\n"

correct would be:

irb> YAML.dump({"foo"=> "No"})
=> "---\nfoo: 'No'\n"

It's already correct for lowercase 'no':

iex> Ymlr.document!(%{foo: "no"})
"---\nfoo: 'no'\n"

Link to documentation

https://yaml.org/type/bool.html

@Zinggi
Copy link
Contributor Author

Zinggi commented Mar 12, 2024

However, it seems like yaml 1.2 dropped this insane handling of boolean values and only accepts true|false.
So which version of the spec does this library actually target?

At the very least, it's an inconsistency, as it handles lowercase no like yaml 1.1, but uppercase like yaml 1.2.

@Zinggi
Copy link
Contributor Author

Zinggi commented Mar 12, 2024

As for me, I think I will switch to using https://github.com/processone/fast_yaml for my project, as I need 1-to-1 compatibility with the ruby yaml library. And since fast_yaml uses the same underlying libyaml, it should provide max compatibility between the two.

@Zinggi
Copy link
Contributor Author

Zinggi commented Mar 12, 2024

fast_yaml seems to handle this case correctly, but unfortunately I get the "wrong" quotes. Not great either for my usecase...

iex> :fast_yaml.encode(%{foo: "No"}) |> IO.iodata_to_binary
"foo: \"No\""

@mruoss
Copy link
Collaborator

mruoss commented Mar 12, 2024

Hi @Zinggi

Thanks for raising this.

So which version of the spec does this library actually target?

The quoted version is valid in both versions. So let's aim for that.

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 a pull request may close this issue.

2 participants