-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
YAML fixes #4838
YAML fixes #4838
Changes from all commits
d55225d
f3c89b4
b607b65
b6a2e09
f14198b
8346108
016dfd4
d6886fc
d11fd05
1bc96b5
ffccba4
de2a1b7
d2476e2
922fa83
47ee91c
86f3f4b
1faeb45
f5fc600
0c605a8
cf95b76
5a90882
b3f6436
3157fab
ceb9db9
e3df0d6
50d5705
e2c8a1a
69ee625
31ca301
6a5b820
7a36cdc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,357 @@ | ||
require "spec" | ||
require "yaml" | ||
|
||
private struct ObjectStub | ||
getter called : Bool | ||
|
||
def self.new(pull : YAML::PullParser) | ||
pull.read_scalar | ||
new(true) | ||
end | ||
|
||
def initialize(@called) | ||
end | ||
end | ||
|
||
describe "Object.from_yaml" do | ||
it "should create call .new on with a YAML::PullParser" do | ||
ObjectStub.from_yaml("hello").called.should be_true | ||
end | ||
end | ||
|
||
describe "Nil.from_yaml" do | ||
it "should serialize and deserialize" do | ||
Nil.from_yaml(nil.to_yaml).should eq nil | ||
end | ||
|
||
values = YAML::NULL_VALUES | ||
|
||
it "should return nil if a YAML null value" do | ||
values.each do |value| | ||
# Test with an array since a standalone empty value shows as STREAM_END | ||
Array(Nil).from_yaml("- " + value).should eq [nil] | ||
end | ||
end | ||
|
||
it "should raise if a YAML null value is quoted" do | ||
values.each do |value| | ||
expect_raises(YAML::ParseException) do | ||
Nil.from_yaml %("#{value}") | ||
end | ||
expect_raises(YAML::ParseException) do | ||
Nil.from_yaml "'#{value}'" | ||
end | ||
end | ||
end | ||
|
||
it "should raise if not a null value" do | ||
expect_raises(YAML::ParseException) do | ||
Nil.from_yaml "hello" | ||
end | ||
end | ||
end | ||
|
||
describe "Bool.from_yaml" do | ||
describe "true values" do | ||
it "should serialize and deserialize" do | ||
Bool.from_yaml(true.to_yaml).should eq true | ||
end | ||
|
||
values = YAML::TRUE_VALUES | ||
|
||
it "should return true if a truthy value" do | ||
values.each do |value| | ||
Bool.from_yaml(value).should eq true | ||
end | ||
end | ||
|
||
it "should raise if a truthy value is quoted" do | ||
values.each do |value| | ||
expect_raises(YAML::ParseException) do | ||
Bool.from_yaml %("#{value}") | ||
end | ||
expect_raises(YAML::ParseException) do | ||
Bool.from_yaml %('#{value}') | ||
end | ||
end | ||
end | ||
end | ||
|
||
describe "false values" do | ||
it "should serialize and deserialize" do | ||
Bool.from_yaml(false.to_yaml).should eq false | ||
end | ||
|
||
values = YAML::FALSE_VALUES | ||
|
||
it "should return true if a falsey value" do | ||
values.each do |value| | ||
Bool.from_yaml(value).should eq false | ||
end | ||
end | ||
|
||
it "should raise if a falsey value is quoted" do | ||
values.each do |value| | ||
expect_raises(YAML::ParseException) do | ||
Bool.from_yaml %("#{value}") | ||
end | ||
end | ||
values.each do |value| | ||
expect_raises(YAML::ParseException) do | ||
Bool.from_yaml %('#{value}') | ||
end | ||
end | ||
end | ||
end | ||
|
||
it "should raise if not a bool value" do | ||
expect_raises(YAML::ParseException) do | ||
Bool.from_yaml "hello" | ||
end | ||
end | ||
end | ||
|
||
[Int8, Int16, Int32, Int64, UInt8, UInt16, UInt32, UInt64].each do |type| | ||
describe "#{type.name}.from_yaml" do | ||
it "should serialize and deserialize" do | ||
type.from_yaml(type.new("1").to_yaml).should eq type.new("1") | ||
end | ||
|
||
it "should parse possible values" do | ||
values = {"1", "-1", "0x_0A_74_AE", "0b1010_0111_0100_1010_1110", "02472256"} | ||
values.each do |value| | ||
type.from_yaml(value).class.should eq type | ||
end | ||
end | ||
|
||
it "should parse a number into an #{type.name}" do | ||
type.from_yaml("1").should eq type.new("1") | ||
end | ||
|
||
it "should raise if an int is quoted" do | ||
expect_raises(YAML::ParseException) do | ||
type.from_yaml %("1") | ||
end | ||
expect_raises(YAML::ParseException) do | ||
type.from_yaml %('1') | ||
end | ||
end | ||
end | ||
end | ||
|
||
[Float32, Float64].each do |type| | ||
describe "#{type.name}.from_yaml" do | ||
it "should serialize and deserialize" do | ||
type.from_yaml(type.new("1.0").to_yaml).should eq type.new("1.0") | ||
end | ||
|
||
it "should parse possible values" do | ||
values = { | ||
".inf", ".Inf", ".INF", "+.inf", "+.Inf", "+.INF", | ||
"-.inf", "-.Inf", "-.INF", | ||
".nan", ".NaN", ".NAN", | ||
"+1", "1", "-1", | ||
"1.1", "+1.1", "-1.1", | ||
} | ||
values.each do |value| | ||
type.from_yaml(value).class.should eq type | ||
end | ||
end | ||
|
||
it "should parse a number into an #{type.name}" do | ||
type.from_yaml("1").should eq type.new("1") | ||
end | ||
|
||
it "should parse a float into an #{type.name}" do | ||
type.from_yaml("1.1").should eq type.new("1.1") | ||
end | ||
|
||
it "should raise if a float value is quoted" do | ||
expect_raises(YAML::ParseException) do | ||
type.from_yaml %("1.1") | ||
end | ||
expect_raises(YAML::ParseException) do | ||
type.from_yaml %('1.1') | ||
end | ||
end | ||
end | ||
end | ||
|
||
describe "String.from_yaml" do | ||
it "should serialize and deserialize" do | ||
String.from_yaml("hi".to_yaml).should eq "hi" | ||
end | ||
|
||
it "should parse a string" do | ||
String.from_yaml("hello").should eq "hello" | ||
end | ||
|
||
it "should parse a quoted string" do | ||
String.from_yaml(%("hello")).should eq "hello" | ||
String.from_yaml(%('hello')).should eq "hello" | ||
end | ||
|
||
it "should parse a literal string" do | ||
String.from_yaml("|\n hello").should eq "hello" | ||
end | ||
|
||
it "should parse a folded string" do | ||
String.from_yaml(">\n hello").should eq "hello" | ||
end | ||
|
||
context "reserved values" do | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm thinking about splitting these reserved values up and cover each set in the context of the specific type. It's not an inherent responsibility of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think that "1.0" is a valid yaml string, unless quoted. So why should it not be string's responsibility to detect that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is only an invalid string because it is a float value format. If the parser schema wouldn't include float type (like the Failsafe Schema), There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see anywhere in the spec where it makes clear how the parser should handle situations like this. Maybe it is a string in the failsafe schema but we don't have an option to select schema do we? If we're not in the fail safe scheme then surely reading a string which is a valid float, which is unquoted, is incorrect. The way that yaml is parsed should be entirely independent of the way that its converted into crystal types. A certain value shouldn't be parsed one way of were expecting a string and the other way if we're expecting a float. Doing so is leaking details of the parsing. The fact is that 99% of yaml parsers will parse an unquoted 1.0 as a number. If we don't do the same, then our parser is incorrect. That leads me to the conclusion that the way we're doing parsing is incorrect. We should be parsing tokens and deciding what type they are inside a single next method. We shouldn't be attempting to read a string and erroring if we get something which could be a float, we should have a single next method which returns the union of all yaml types, and parses the scalar to a concrete type irrespective of what type we've told the pullparser we're expecting. This is the same way it's done in the json pullparser. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @RX14 I think these general considerations shouldn't go into a line comment. Will get lost easily this way... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It all depends on the used schema. If a parser supports Core Schema, it will of course recognize a float value. If the schema has no float, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, but we do recognize float/core schema. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From a logical perspective, it's not a property of the string parser to recognize There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This way it's easier to track for example further additions to the parser which might come in the future because these, too, would be introduced in one piece of matching values and non-matching string values. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What i'm suggesting is to move all the scalar parsing logic into We can update the parser to have selectable schemas later. (failsafe schema will just error if you try to read a float for example) |
||
values = { | ||
"true", "True", "TRUE", "on", "On", "ON", "y", "Y", "yes", "Yes", "YES", | ||
"false", "False", "FALSE", "off", "Off", "OFF", "n", "N", "no", "No", "NO", | ||
"", "NULL", "Null", "null", "~", | ||
".inf", ".Inf", ".INF", "+.inf", "+.Inf", "+.INF", | ||
"-.inf", "-.Inf", "-.INF", | ||
".nan", ".NaN", ".NAN", | ||
"+1", "1", "-1", | ||
"1.1", "+1.1", "-1.1", | ||
"0x_0A_74_AE", "0b1010_0111_0100_1010_1110", "02472256", | ||
"2001-12-15T02:59:43.1Z", | ||
} | ||
|
||
it "should raise if a reserved value" do | ||
values.each do |value| | ||
expect_raises(YAML::ParseException) do | ||
String.from_yaml(value) | ||
end | ||
end | ||
end | ||
|
||
it "should parse if a reserved value is quoted" do | ||
values.each do |value| | ||
String.from_yaml(%("#{value}")).should eq value | ||
end | ||
values.each do |value| | ||
String.from_yaml(%('#{value}')).should eq value | ||
end | ||
end | ||
end | ||
end | ||
|
||
describe "Array.from_yaml" do | ||
it "should serialize and deserialize" do | ||
Array(String).from_yaml(["hi"].to_yaml).should eq ["hi"] | ||
end | ||
|
||
it "it should parse an array of the correct objects" do | ||
result = Array(String).from_yaml <<-YAML | ||
- one | ||
- two | ||
- three | ||
YAML | ||
result.should eq ["one", "two", "three"] | ||
end | ||
|
||
context "with a union type" do | ||
it "it should parse an array of the correct objects" do | ||
result = Array(String | Int32 | Float64).from_yaml <<-YAML | ||
- one | ||
- 1 | ||
- 1.0 | ||
YAML | ||
result.should eq ["one", 1, 1.0] | ||
end | ||
end | ||
end | ||
|
||
describe "Hash.from_yaml" do | ||
it "should serialize and deserialize" do | ||
Hash(String, String).from_yaml({"hello" => "world"}.to_yaml).should eq ({"hello" => "world"}) | ||
end | ||
|
||
it "it should parse an array of the correct objects" do | ||
result = Hash(String, String).from_yaml <<-YAML | ||
foo: one | ||
bar: two | ||
baz: three | ||
YAML | ||
result.should eq ({"foo" => "one", "bar" => "two", "baz" => "three"}) | ||
end | ||
|
||
context "with a union type" do | ||
it "it should parse an array of the correct objects" do | ||
result = Hash(String, String | Int32 | Float64).from_yaml <<-YAML | ||
foo: 1 | ||
bar: two | ||
baz: 3.0 | ||
YAML | ||
result.should eq ({"foo" => 1, "bar" => "two", "baz" => 3.0}) | ||
end | ||
end | ||
end | ||
|
||
describe "Tuple.from_yaml" do | ||
it "should serialize and deserialize" do | ||
Tuple(String, String).from_yaml({"hello", "world"}.to_yaml).should eq ({"hello", "world"}) | ||
end | ||
|
||
it "it should parse an array of the correct objects" do | ||
result = Tuple(String, String, String).from_yaml "- one\n- two\n- three" | ||
result.should eq ({"one", "two", "three"}) | ||
end | ||
|
||
context "with a union type" do | ||
it "it should parse an array of the correct objects" do | ||
result = Tuple(String | Int32 | Float64, String | Int32 | Float64, String | Int32 | Float64).from_yaml "- one\n- 1\n- 1.0" | ||
result.should eq ({"one", 1, 1.0}) | ||
end | ||
end | ||
end | ||
|
||
describe "NamedTuple.from_yaml" do | ||
it "it should parse an array of the correct objects" do | ||
result = NamedTuple(foo: String, bar: String, baz: String).from_yaml "foo: one\nbar: two\nbaz: three" | ||
result.should eq ({foo: "one", bar: "two", baz: "three"}) | ||
end | ||
end | ||
|
||
enum TestEnum | ||
Red | ||
Green | ||
Blue | ||
end | ||
|
||
describe "Enum.from_yaml" do | ||
it "should serialize and deserialize" do | ||
TestEnum.from_yaml(TestEnum::Red.to_yaml).should eq TestEnum::Red | ||
end | ||
|
||
it "should parse from string" do | ||
TestEnum.from_yaml("red").should eq TestEnum::Red | ||
end | ||
|
||
it "should parse from int" do | ||
TestEnum.from_yaml("1").should eq TestEnum::Green | ||
end | ||
end | ||
|
||
describe "Union.from_yaml" do | ||
it "should compile into any of the types" do | ||
(Bool | Int32).from_yaml("true").should eq true | ||
(Bool | Int32).from_yaml("1").should eq 1 | ||
end | ||
|
||
it "should raise if not a unions type" do | ||
expect_raises(YAML::ParseException) do | ||
(Bool | Int32).from_yaml("foo") | ||
end | ||
end | ||
end | ||
|
||
describe "Time.from_yaml" do | ||
it "should parse time" do | ||
time = Time.now | ||
time_string = Time::Format::ISO_8601_DATE_TIME.format(time) | ||
Time.from_yaml(time_string).epoch.should eq time.epoch | ||
end | ||
|
||
it "should not parse an invalid time string" do | ||
expect_raises(YAML::ParseException) do | ||
Time.from_yaml("not a time") | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add the same line for
false
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see it's in the next spec group.