Fix handling of quoted boolean values in YAML::Any
#13546
Conversation
spec/std/yaml/any_spec.cr
Outdated
| YAML.parse("yes").as_bool?.should be_true | ||
| YAML.parse("no").as_bool?.should be_false | ||
| YAML.parse("'yes'").as_bool?.should be_nil | ||
| YAML.parse("'no'").as_bool?.should be_nil |
There was a problem hiding this comment.
These specs pass even without this PR. Instead this PR seems to only affect users of YAML::Serializable::Unmapped:
require "yaml"
class Foo
include YAML::Serializable
include YAML::Serializable::Unmapped
end
# before
Foo.from_yaml("x: yes") # => #<Foo:0x13732ca0ce0 @yaml_unmapped={"x" => true}>
Foo.from_yaml("x: 'yes'") # => #<Foo:0x13732ca0bc0 @yaml_unmapped={"x" => true}>
YAML.parse("'yes'").as_bool? # => nil
# after
Foo.from_yaml("x: yes") # => #<Foo:0x24f0b1d0cc0 @yaml_unmapped={"x" => true}>
Foo.from_yaml("x: 'yes'") # => #<Foo:0x24f0b1d0ba0 @yaml_unmapped={"x" => "yes"}>
YAML.parse("'yes'").as_bool? # => nilThere was a problem hiding this comment.
Ah YAML.parse and YAML::Any.from_yaml seem to go through different codepaths?
I can't see any specs specifically for YAML::Any.from_yaml—should I add some in this file?
And yeah this will change the parsing of YAML::Unmapped or if you have a field of type YAML::Any (which is what I was doing).
So:
require "yaml"
# Before
YAML::Any.from_yaml "'yes'" # => true
YAML::Any.from_yaml "yes" # => true
YAML.parse "'yes'" # => "yes"
YAML.parse "yes" # => true
# After
YAML::Any.from_yaml "'yes'" # => "yes"
YAML::Any.from_yaml "yes" # => true
YAML.parse "'yes'" # => "yes"
YAML.parse "yes" # => trueThere was a problem hiding this comment.
Please do add the necessary specs
There was a problem hiding this comment.
Ah
YAML.parseandYAML::Any.from_yamlseem to go through different codepaths?
Yeah, that's all a bit weird and hard to follow through, what's actually going on. YAML parsing could probably use some refactoring just to get the code clarified.
`"yes"` and `"no"` should be treated as strings (since they're quoted) whereas `yes` and `no` should be booleans (since they're plain). This is already handled in most places, but `YAML::Any` passes the node value, rather than the node itself when handling a scalar and so the quoting type is lost, and everything is parsed as though it is unquoted.
|
Removed the ref: https://forum.crystal-lang.org/t/breaking-changes/5805/2?u=straight-shoota |
"yes"and"no"should be treated as strings (since they're quoted) whereasyesandnoshould be booleans (since they're plain). This is already handled in most places, butYAML::Anypasses the node value, rather than the node itself when handling a scalar and so the quoting type is lost, and everything is parsed as though it is unquoted.This is a breaking change.