YAML revamp #5007

asterite · 2017-09-19T16:29:03Z

This is a big PR so I'll explain everything in detail.

What's wrong with the current YAML implementation?

Several things:

YAML.parse, YAML.dump and YAML.mapping only support the failsafe schema, when the core schema is used by default in every other programming language. The core schema supports many things, for example "null" will parse to nil, "0b01" will parse as a binary integer, and "2010-01-02" will parse as a time.
Parsing with YAML.mapping, and dumping via YAML.dump/#to_yaml fails on recursive types: anchors and aliases are simply ignored.
Some APIs were missing arguments, notably YAML::Builder to adjust the style, anchors and tags of sequences, mappings and scalars.
Most of the API was undocumented.

This PR fixes all of that.

I'll explain what changed in each of the files in this PR, but first I'll give a general overview.

General overview

YAML.parse now delegates to YAML::Schema::Core::Parser.parse. The idea is that all logic related to the core schema is under YAML::Schema::Core. This lets YAML::PullParser keep its simplicity.
YAML::Builder and YAML::PullParser can track anchors, aliases and recursive data structures. At first I thought about making a layer on top of these types. However, anchors, aliases and recursive types are a core part of the YAML specification, and an implementation can't simply ignore that. That's why parsing and emitting YAML must take that into account.

File by file explanation

You can use this explanation as you traverse the diff (same order as there).

`spec/std/yaml/any_spec.cr`

YAML::Any now wraps more types, for example Int64, Float64 and time, so methods to access these values need to be tested.

Additionally, YAML mappings (Hash in Crystal) can have complex keys, not just strings. That's why we should be able to index with 1 into a Hash.

A few specs changed because what previously returned a String now returns an Int.

`spec/std/yaml/builder_spec.cr`

YAML::Builder can now be specified the styles, anchors and tags of mappings, sequences and scalars, and this is tested.

`spec/std/yaml/mapping_spec.cr`

YAML.mapping can now serialize and deserialize recursive structures, and this is tested

`spec/std/yaml/schema/core_spec.cr`

Here we test parsing scalars into the different types (Nil, Bool, Int64, Float64, Time, etc.) according to the core schema. The nice thing about this is that this can almost be tested independently of the other YAML classes and structs.

`spec/std/yaml/serialization_spec.cr`

#to_yaml and YAML.dump now use the core schema by default, so some variants of the different types (like null and Null for nil) are tested. Not every variant is tested because this is already covered in core_spec.cr.

Additionally we test that recursive arrays and hashes can be serialized.

`spec/std/yaml/yaml_spec.cr`

Some tests changed because of the core schema. I didn't add more specs to test that, for example, "~" parses into nil because that is covered in core_spec.cr. If we assume the parsing delegates to the core schema parser it's a bit redundant to test this twice.

`src/yaml.cr`

Clarify in the docs that the core schema is used, and change YAML::Type accordingly.

`src/yaml/any.cr`

See the explanation for any_spec.cr

`src/yaml/builder.cr`

See the explanation for builder_spec.cr.

Additionally, the builder now tracks info to be able to dump recursive data. This is all explained in the comments.

If you implement to_yaml for a Reference type you need to use register when needed. However, to_yaml is implemented for the basic Crystal types, and YAML.mapping provides a correct implementation, so needing to learn this should be pretty rare (but it's still explained).

`src/yaml/enums.cr`

I moved the enums from LibYAML into the YAML namespace, so that wan can more easily use them without needing to know about LibYAML.

`src/yaml/from_yaml.cr`

Implementation of from_yaml for many Crystal types.

Most of this is done with the helper method parse_scalar in that same file. We simply parse a scalar according to the core schema from the pull parser, and if it's of the type we were trying to deserialize, we return it. Otherwise it's an error.

This might seem a bit slow, to parse a scalar for example to deserialize nil. However, parse_scalar doesn't allocate memory, so it's fast. I include some benchmarks at the end.

Some other changes include tracking of types to allow recursiveness.

`src/yaml/lib_yaml.cr`

Changed because the enums were moved to the YAML schema.

`src/yaml/mapping.cr`

Changed to support recursion.

To implement this is a bit tricky. First, we can't implement this just with initialize because we want to return a different object if we find it in the anchors. So, we need to define a new method. However, right now the compiler doesn't allow both a new and an initialize with the same argument "overloadness": the initialize wins and the new is hidden. This might be fixed later. For now, to make them different overloads, I added a _dummy argument at the end.

`src/yaml/parser.cr`

Now the Parser is an abstract class to possibly support more schemas. Reads the docs to understand how. There are also two implementations with this: failsafe and core.

According to YAML's spec there are three recommended schemas:

For Failsafe the spec says:

A YAML processor should therefore support this schema, at least as an option.

That's why I see no reason not to support this, specially since it's so easy with how YAML::Parser was refactored.

Ideally I'd like to support JSON schema too, shouldn't be that hard, except I don't understand well the spec 😊

`src/yaml/pull_parser.cr`

This type had a few changes, some breaking changes too:

Added doc comments for almost every method
Added support for tracking recursive types
Unified a few methods (like anchor)
Use .foo? instead of the long enum name when possible

To support recursive types I use a a bit of low-level knowledge of Crystal, mainly object_id and crystal_type_id.

`src/yaml/schema/core.cr`

This implements all the logic related to the core schema. Provides a few public methods, and a parser.

To parse a scalar no Regex is used. And no memory is allocated. It's fast. I'll show a benchmark in the end.

`src/yaml/schema/core/time_parser.cr`

Specialized time parser. See the docs for why I didn't use Time.parse.

`src/yaml/schema/fail_safe.cr`

Failsafe implementation. Doesn't use YAML::Any. Might be useful to some (for example shards could use this if it previously didn't need the core schema).

`src/yaml/to_yaml.cr`

Updated to support recursive types. Also changed String to emit double quoted for reserved scalars in the core schema. And for Time there's custom logic to use the nicest output format.

How is this different than #4838

First, I'd like to thank you, @jwaldrip, because you was the first who decided to start doing something about this missing YAML feature. The code is great, but I personally imagined it organized a bit differently. In the process I looked at your code (I even copied most of YAML::Any from it).

So, this PR is similar to #4838 but with a few differences:

Code organization (code, but also types) is better (in my opinion) and more extensible (for example allowing to support other schemas)
YAML fixes #4838 doesn't support the full core schema (for example only ISO8601 timestamps are supported, while this PR supports all variants)
YAML fixes #4838 is just a bit slower (it's actually pretty fast, but this PR is a bit faster)

I'd still like to thank @jwaldrip in the changelog entry because without it, I wouldn't have done anything (no pressure to provide an implementation).

Benchmarks

I used this benchmark:

require "yaml"

yaml = %(
int: 1
int2: -123
int3: +456
int4: 123_456
int5: -123_456
octal: 0123
hex: 0xabCDef
binary: 0b101011
float: 1.2
float: -1.3e10
float2: +12.34e2
float3: .inf
float4: -.INF
float5: .nan
float6: .NaN
float7: -1_234.567_891
bool: true
another_bool: false
date: 2002-12-14
time: 2002-1-2T10:11:12
str: hello world
str2: 1234567890hello
tag_str: !!str 1
)

value = YAML.parse(yaml)

time = Time.now
1_000.times do
  YAML.dump(value)
end
print "Time to dump 1_000 times: "
puts Time.now - time

time = Time.now
10_000.times do
  value = YAML.parse(yaml)
end
print "Time to parse 10_000 times: "
puts Time.now - time

I get:

Time to dump 1_000 times: 00:00:00.0341950
Time to parse 10_000 times: 00:00:00.5267270

We can run almost the same code in Ruby. Make sure to change parse to load because parse just builds a YAML nodes tree. In Ruby I get:

Time to dump 1_000 times: 0.58858
Time to parse 10_000 times: 4.720954

So more than 10x faster to dump and parse.

Can we do it faster? Maybe. But for me, if we are already much faster than Ruby it's enough (if this parsing speed is good-enough for Ruby, for us it's more than good-enough)

Fixes #4384
Fixes #1745
Fixes #2873
Fixes #3101
Fixes #4929
Closes #4838

luislavena · 2017-09-19T17:16:48Z

src/yaml/from_yaml.cr: I include some benchmarks at the end.

I couldn't find any of the benchmarks in that file or as part of the commit message. Can you add that to the PR?

Thank you.

asterite · 2017-09-19T17:17:21Z

@luislavena I hit "submit" before time. I'm still editing the description :)

asterite · 2017-09-19T17:29:35Z

I updated the description. This is now ready to review 😄

Sija · 2017-09-19T17:44:10Z

src/yaml/any.cr

  #
-  # Raises if the underlying value is not an `Array`.
-  def [](index : Int) : YAML::Any
+  # Raises if the underlying value is not an `Array` nor a `Has.


[...] nor a `Has.

->

[...] nor a Hash.

Sija · 2017-09-19T17:44:31Z

src/yaml/any.cr

    else
-      raise "Expected Hash for #[](key : String), not #{object.class}"
+      raise "Expected Array or hash for #[](index_or_key), not #{object.class}"


hash -> Hash

Sija · 2017-09-19T17:49:24Z

src/yaml/from_yaml.cr

+      {% end %}
+    {% end %}
+
+    pull.raise("error deserailizing alias")


Capitalize exception message?

Sija · 2017-09-19T17:51:56Z

src/yaml/pull_parser.cr

+        end
+      end
+
+      raise("error deserailizing alias") if raise_on_alias


Capitalize exception message?

Sija · 2017-09-19T17:52:45Z

src/yaml/enums.cr

@@ -0,0 +1,36 @@
+module YAML


Why not leave 'em in LibYAML and use alias instead of moving all the enums?

Because API docs don't show for lib types.

Sija · 2017-09-19T17:54:35Z

src/yaml/mapping.cr

+        return obj
+      end
+
+      instance = allocate


Shouldn't instance be a fresh variable? Similarly obj above.

Not necessarily. There's no risk of clashing other variables.

Sija · 2017-09-19T17:55:32Z

src/yaml/mapping.cr

+      instance
+    end
+
+    def initialize(%pull : ::YAML::PullParser, _dummy : Nil)


What's the purpose of _dummy argument here?

Just from description:

However, right now the compiler doesn't allow both a new and an initialize with the same argument "overloadness": the initialize wins and the new is hidden. This might be fixed later. For now, to make them different overloads, I added a _dummy argument at the end.

Fair 'nuff, could we have TODO or FIXME comment then to keep those edgy cases at bay?

Sija · 2017-09-19T17:57:38Z

src/yaml/pull_parser.cr

+
+  private def record_anchor(object_id, crystal_type_id)
+    anchor = self.anchor
+    return unless anchor


Why not simplify it to:

return unless anchor = self.anchor

I find it harder to understand like that.

Sija · 2017-09-19T18:22:13Z

src/yaml/any.cr

      value ? Any.new(value) : nil
    else
-      raise "Expected Hash for #[]?(key : String), not #{object.class}"
+      raise "Expected Array for #[]?(index : Int), not #{object.class}"


Expected Array for #[]?(index : Int), not #{object.class}

->

Expected Array or Hash for #[]?(index_or_key), not #{object.class}

jwaldrip · 2017-09-20T03:54:50Z

src/yaml/schema/core.cr

+  # ```
+  def self.parse_scalar(string : String) : Type
+    case string
+    when .empty?


Why not put this with the other null checks?

when "~", "null", "Null", "NULL", .empty?

My guess is to make it more readable.

Oh, I totally forgot I could write it like that. I'll change it :-)

jwaldrip · 2017-09-20T04:03:17Z

Great job @asterite. The code reorganization looks good. More than anything, I am just happy to see these fixes get in. Was support added for !!set and !!omap from the YAML spec too? Also, I think we should refer to the spec by it's version rather than Core. As far as I can tell, this adheres to the 1.1 spec, but we could add the 1.2 spec at a later date, so that naming may make more sense.

asterite · 2017-09-20T13:25:05Z

@jwaldrip Do you know what's the difference between the YAML 1.1 and 1.2 specs? Maybe the regexes for the core type?

I can see here that booleans are just variants of true and false, there's no yes nor no. Is that the difference? Also base 8 starts with 0o (zero o) instead of 0 (zero).

I think I like version 1.2 better because it's simpler. It's a bit strange to deserialize "yes" as true. In fact Ruby deserializes "y" to "y", while the 1.1 spec says it should be true.

But I don't know if that's the difference. Is there anything else?

(in any case, you are totally right that we should mention which version we support)

About the tags:

!!omap: the default YAML mapping has no order guaranteed, while !!omap should deserialize to something that preserves insertion order. But in Crystal Hash already preserves order so I don't think there's something special we need to do here.
!!set: should we deserialize this to a Set? Seems pretty easy to implement. However, in Ruby I can see it deserializes to a Hash with nil values. The spec suggests that's also possible. I don't understand why Ruby chose a Hash when there's Set in the standard library. Do you know the reason? In any case I can push an update with !!set support and use Set.

asterite · 2017-09-20T13:37:38Z

To a bit a more to the above: I'd really prefer to implement 1.2, making "y", "yes", "on", etc., be booleans is super strange, given that JSON is so popular and true and false are ubiquitous.

However, YAML 1.2 doesn't support timestamp, right? So we shouldn't deserialize Time at all?

asterite · 2017-09-20T14:29:43Z

Actually, we are using libyaml and that's a 1.1 parser, so we'll stick with 1.1 for now.

I'll soon add support for all types defines here.

I also checked pyyaml (which uses libyaml) and they also implicitly parse times like in this PR and like in Ruby's Psych implementation, so I guess it's safe if we do the same thing.

luislavena · 2017-09-20T16:45:02Z

Do you know the reason? In any case I can push an update with !!set support and use Set.

@asterite, Even that Set is present in stdlib of Ruby, is not part of Core. If not mistaken, the decision was to avoid automatically require a data type (Set) that might not be used by developers and instead map to core values.

Cheers.

If I can chime in, I would definitely like !!set to be mapped to a proper Set instance instead of hash-for-everything approach of Ruby 😁

Thank you for working on this! ❤️ ❤️ ❤️

jwaldrip · 2017-09-20T19:26:10Z

Also !!pairs should probably be Array(NamedTuple(key: value)) and !!omap should just force ordering on the keys.

asterite · 2017-09-20T19:55:35Z

I actually tried to make !!pairs be Array(Tuple(Type, Type)) but it complicates things a lot because now we have arrays that either have other yaml types or a tuple (or named tuple in your case). I decided to parse it to an array of single-element hashes, like in Ruby. I also don't think !!pairs is common enough (and neither all of the other tags, I've never seen one in my life in real world code).

jwaldrip · 2017-09-20T19:58:00Z

@asterite, array of single element hashes makes sense. Did you happen to implement !!binary? In where !!binary should base64 encode into Bytes.

asterite · 2017-09-20T20:10:56Z

@jwaldrip Yes, it decodes to Bytes :-)

asterite · 2017-09-20T20:44:10Z

Hmmm... I managed to implement everything (not pushed yet) except merge. Well, merge actually works with YAML.parse but it doesn't work with YAML.mapping, and it would be good if it worked.

I'll explain the difficulty. For example this:

development: &development
  host: foo.com
  port: 1234

test:
  <<: *development
  port: 4567

With this:

require "yaml"

class HTTPSetting
  YAML.mapping(name: String, port: Int32)
end

We want to Array(HTTPSetting).from_yaml.

The way it currently works is that when &development is found, just after initializing an HTTPSetting for it we associate it with the anchor "development". Then when *development is found we would retrieve it... but then? We'd somehow have to traverse its instance vars and pull from that instead of from a pull parser. It gets a bit worse if *development doesn't even point to another instance of HTTPSetting (could be another type with less properties).

So I'm thinking the only solution to this is to parse YAML first to an intermediate structure, and YAML.mapping would work on that structure. So that intermediate structure will have a YAML tree associated to &development, and when we find <<: *development we would get that tree and traverse its key/values as usual (the main issue of doing it with a pull parser is that we lost the information of what was under &development, we only have the Crystal deserialization).

This implies a medium-sized refactor, and of course it's a breaking change. It will also slow down parsing a bit because of the intermediate structure, but I don't think it will get that bad. But it's better to be feature complete than fast but incomplete.

That is, unless someone has a better idea of how to solve this issue :-)

Sija · 2017-09-20T23:05:19Z

src/yaml/any.cr

      value ? Any.new(value) : nil
    else
-      raise "Expected Hash for #[]?(key : String), not #{object.class}"
+      raise "Expected Array or Hash for #[]?(index : Int), not #{object.class}"


Argument declaration should be index_or_int instead of index : Int.

Sija · 2017-09-20T23:10:17Z

src/yaml/pull_parser.cr

  def read_null_or
-    if kind == EventKind::SCALAR && (value = self.value).nil? || (value && value.empty?)
+    if kind == EventKind::SCALAR && scalar_style.plain? && value.empty?


if kind.scalar? && ...

?

Sija · 2017-09-20T23:11:28Z

src/yaml/pull_parser.cr

    expect_kind expected_kind
    read_next
  end

  def read_raw
    case kind
-    when EventKind::SCALAR
+    when .scalar?
      self.value.not_nil!.tap { read_next }


.not_nil! call here is no longer needed.

Sija · 2017-09-20T23:11:38Z

src/yaml/pull_parser.cr

@@ -188,25 +320,25 @@ class YAML::PullParser

  def read_raw(io)
    case kind
-    when EventKind::SCALAR
+    when .scalar?
      self.value.not_nil!.inspect(io)


RX14 · 2017-09-21T21:08:35Z

@asterite surely we only have to store the YAML structures which are tagged?

asterite · 2017-09-21T21:16:03Z

So... this PR is now really big, but all the changes are needed to correctly
implement the full YAML 1.1 spec with the custom types, include merge ("<<").

(Conclusion: YAML is hard, but also fun ^_^)

At this point, I think it's better to browse the implementation, or the specs,
to see that everything works as expected. I did my best to document most
of the public API and some internal details and algorithms.

Parsing and dumping YAML is now done in a completely different way.

For parsing (with from_yaml), we first parse from a String or IO
into an in-memory data structure. We then convert this data structure
to the appropriate Crystal types, with these types implementing a new
method that accept two arguments: a YAML::ParseContext and a
YAML::Nodes::Node that is the node to deserialize. The parse context
is used to store and retrieve references for anchors and aliases.

For parsing with YAML.parse we can avoid this intermediate data structure
and directly parse it into the final structure because it's much simpler.

For dumping, we have objects implement to_yaml with an argument of
YAML::Nodes::Builder. This builder, instead of directly outputting stuff
to an IO, will create an in-memory tree. This tree can be linked with anchors
and aliases. That way to_yaml is invoked just once on every object. Then,
we turn that in-memory tree into proper YAML.

By the way, this is also how Ruby does it, probably because of the same
reasons.

This should be slightly slower than before, and a bit more memory will be
allocated to create the intermediate data structure, but it's worth it
because many more features can be implemented, and the implementation turns
out to be simpler like this.

One of the features this refactor supports is merging with "<<". For example,
this now works:

require "yaml"

class Setting
  YAML.mapping(host: String, port: Int32)
end

yaml = <<-YAML
development: &development
  host: foo.com
  port: 1234

test:
  <<: *development
  port: 4567
YAML

settings = Hash(String, Setting).from_yaml(yaml)
p settings
# => 
# {"development" => #<Setting:0x106f7fd80 @host="foo.com", @port=1234>,
#  "test" => #<Setting:0x106f7fd20 @host="foo.com", @port=4567>}

This works because when the implementation of YAML.mapping finds
a << key, it can solve the alias to a node and traverse that
node's keys and values to continue mapping from there (recursively!).
I believe this is something that must be implemented as this usage
of the merge key is pretty common to override settings from a common
setting pool.

Another advantage is that when deserializing a union of types there's
no need to use PullParser#read_raw and build a value from it and
parse it multiple times, retrying on failure: we can just parse
from the already parsed in-memory node. So this should actually be
faster than before.

Finally, here's the updated benchmark result:

Time to dump 1_000 times: 00:00:00.0395070
Time to parse 10_000 times: 00:00:00.5813970

The previous one was:

Time to dump 1_000 times: 00:00:00.0341950
Time to parse 10_000 times: 00:00:00.5267270

We can see it's a bit slower, but still around 9~10 times faster than
Ruby, so more than good enough :-)

asterite · 2017-09-21T21:17:51Z

Oh, I almost forgot: all YAML types are now implemented, for example binary to encode and decode Bytes.

akzhan · 2017-09-21T21:34:07Z

src/yaml/from_yaml.cr

+def Union.new(ctx : YAML::ParseContext, node : YAML::Nodes::Node)
+  if node.is_a?(YAML::Nodes::Alias)
+    {% for type in T %}
+      {% if type < ::Reference %}


can you describe this trick? looks like you want !type.is_a?(Reference)

It means "if the type inherits from Reference". If so, we can check for an alias.

looks like Crystal need explicit method named .reference??

Yes, but note that this only works in macros. In runtime this isn't possible (yet)

asterite · 2017-09-21T22:03:43Z

Now fixes #2873 too :-)

Sija · 2017-09-21T22:19:32Z

@asterite Other ones that could be related: #4929, #3101 & #4668

- Add support for YAML core schema - Add support for possibly other schemas via the abstract YAML::Parser class - Add missing arguments to YAML::Builder (tag, anchor, scalar) - Add support for parsing and generating recursive data structures - Add support for merge (<<) - Move LibYAML enums to the YAML namespace

asterite · 2017-09-21T22:39:26Z

I decided to remove YAML::PullParser#read_raw because it's not needed anymore to correctly parse a Union. That means #4929 is trivially fixed.

akzhan · 2017-09-23T22:52:52Z

LGTM

jwaldrip · 2017-09-26T14:17:08Z

LGTM

straight-shoota · 2017-09-27T11:14:26Z

Looks really great, thanks @asterite and @jwaldrip for the hard work!

Just as a note: We should consider applying an independent YAML test suite like yaml/yaml-test-suite (though it seems to be incomplete) to make sure that the implementation meets the standard. This doesn't need to be in this PR, obviously.

Sija · 2017-09-27T11:17:51Z

🕚 to 🚢

luislavena · 2017-09-29T20:25:37Z

@asterite @bcardiff @mverzilli merging this before the weekend will be a nice way to end the week! 😄

bcardiff · 2017-09-29T21:05:50Z

src/yaml/from_yaml.cr

-      raise YAML::ParseException.new(ex.message.not_nil!, *location)
-    end
+  def {{type.id}}.new(ctx : YAML::ParseContext, node : YAML::Nodes::Node)
+    {{type.id}}.new parse_scalar(ctx, node, Int64)


@asterite I don't follow how/if UInt64 is supported. The call to parse_scalar uses Int64 to check if the parsed type matches, but UInt64 won't fit.

require "yaml" typeof(YAML::Schema::Core.parse_scalar(UInt64::MAX.to_s)) # => Bool | Float64 | Int64 | String | Time | Nil YAML::Schema::Core.parse_scalar(UInt64::MAX.to_s).class # => Float64

Note that

it_parses_scalar UInt64::MAX.to_s, UInt64::MAX

passes since UInt64::MAX == UInt64::MAX.to_f

I think it's supported if you do UInt64.from_yaml(...), but parse_scalar only returns one of the Type values, so just Int64 is supported (I don't know if it overflows or it just returns a string in this case).

It returns a Float64 now :-)

bcardiff · 2017-09-29T21:07:33Z

src/yaml/to_yaml.cr

-  def to_yaml(yaml : YAML::Builder)
-    yaml.scalar Time::Format::ISO_8601_DATE_TIME.format(self)
+  def to_yaml(yaml : YAML::Nodes::Builder)
+    if kind.utc? || kind.unspecified?


Can't this be encapsulated in a format somehow? It seems pretty useful :-)

Yes! We can probably do that later. Also, right now just 3 decimals are shown but after we merge the PR that makes Time hold nanoseconds we can show all 9 digits.

mverzilli · 2017-10-02T15:49:24Z

The build failed: https://travis-ci.org/crystal-lang/crystal/jobs/282301194

Maybe some conflict between this PR and #5022?

Sija reviewed Sep 19, 2017

View reviewed changes

asterite added kind:feature topic:stdlib labels Sep 19, 2017

asterite self-assigned this Sep 19, 2017

Sija reviewed Sep 19, 2017

View reviewed changes

src/yaml/from_yaml.cr Outdated

{% end %}

{% end %}

pull.raise("error deserailizing alias")

Copy link

Contributor

Sija Sep 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Capitalize exception message?

Sija reviewed Sep 19, 2017

View reviewed changes

src/yaml/pull_parser.cr Outdated

end

end

raise("error deserailizing alias") if raise_on_alias

Copy link

Contributor

Sija Sep 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Capitalize exception message?

Sija reviewed Sep 19, 2017

View reviewed changes

jwaldrip reviewed Sep 20, 2017

View reviewed changes

asterite changed the title ~~YAML revamp~~ [WIP] YAML revamp Sep 20, 2017

Sija reviewed Sep 20, 2017

View reviewed changes

asterite changed the title ~~[WIP] YAML revamp~~ YAML revamp Sep 21, 2017

akzhan reviewed Sep 21, 2017

View reviewed changes

akzhan mentioned this pull request Sep 23, 2017

Added support for mapping YAML nodes not specified in properties #4411

Closed

mverzilli added this to the Next milestone Sep 25, 2017

bcardiff reviewed Sep 29, 2017

View reviewed changes

bcardiff approved these changes Sep 30, 2017

View reviewed changes

mverzilli merged commit 3335450 into crystal-lang:master Oct 2, 2017

straight-shoota mentioned this pull request Oct 2, 2017

Fix for YAML spec failure with new nanoseconds precision #5069

Merged

akzhan mentioned this pull request Oct 2, 2017

YAML/Time parsers/formatters now take in care nanoseconds #5070

Merged

Sija mentioned this pull request Oct 5, 2017

Shards are not working with Crystal 0.24 crystal-lang/shards#175

Closed

asterite added the breaking-change label Oct 14, 2017

jwaldrip mentioned this pull request Jun 29, 2019

Incorrect Parsing of YAML Hash with Union Value #7936

Closed

HertzDevil mentioned this pull request Sep 23, 2023

Fix incorrect overflow in UInt64.from_yaml #13829

Merged

YAML revamp #5007

YAML revamp #5007

Conversation

asterite commented Sep 19, 2017 • edited Loading

What's wrong with the current YAML implementation?

General overview

File by file explanation

spec/std/yaml/any_spec.cr

spec/std/yaml/builder_spec.cr

spec/std/yaml/mapping_spec.cr

spec/std/yaml/schema/core_spec.cr

spec/std/yaml/serialization_spec.cr

spec/std/yaml/yaml_spec.cr

src/yaml.cr

src/yaml/any.cr

src/yaml/builder.cr

src/yaml/enums.cr

src/yaml/from_yaml.cr

src/yaml/lib_yaml.cr

src/yaml/mapping.cr

src/yaml/parser.cr

src/yaml/pull_parser.cr

src/yaml/schema/core.cr

src/yaml/schema/core/time_parser.cr

src/yaml/schema/fail_safe.cr

src/yaml/to_yaml.cr

How is this different than #4838

Benchmarks

luislavena commented Sep 19, 2017 • edited Loading

asterite commented Sep 19, 2017

asterite commented Sep 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwaldrip commented Sep 20, 2017

asterite commented Sep 20, 2017

asterite commented Sep 20, 2017

asterite commented Sep 20, 2017

luislavena commented Sep 20, 2017 • edited Loading

jwaldrip commented Sep 20, 2017 • edited Loading

asterite commented Sep 20, 2017

jwaldrip commented Sep 20, 2017 • edited Loading

asterite commented Sep 20, 2017

asterite commented Sep 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 commented Sep 21, 2017

asterite commented Sep 21, 2017

asterite commented Sep 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asterite commented Sep 21, 2017

Sija commented Sep 21, 2017 • edited Loading

asterite commented Sep 21, 2017

akzhan commented Sep 23, 2017

jwaldrip commented Sep 26, 2017

straight-shoota commented Sep 27, 2017 • edited Loading

Sija commented Sep 27, 2017

luislavena commented Sep 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asterite commented Sep 19, 2017 •

edited

Loading

`spec/std/yaml/any_spec.cr`

`spec/std/yaml/builder_spec.cr`

`spec/std/yaml/mapping_spec.cr`

`spec/std/yaml/schema/core_spec.cr`

`spec/std/yaml/serialization_spec.cr`

`spec/std/yaml/yaml_spec.cr`

`src/yaml.cr`

`src/yaml/any.cr`

`src/yaml/builder.cr`

`src/yaml/enums.cr`

`src/yaml/from_yaml.cr`

`src/yaml/lib_yaml.cr`

`src/yaml/mapping.cr`

`src/yaml/parser.cr`

`src/yaml/pull_parser.cr`

`src/yaml/schema/core.cr`

`src/yaml/schema/core/time_parser.cr`

`src/yaml/schema/fail_safe.cr`

`src/yaml/to_yaml.cr`

luislavena commented Sep 19, 2017 •

edited

Loading

luislavena commented Sep 20, 2017 •

edited

Loading

jwaldrip commented Sep 20, 2017 •

edited

Loading

jwaldrip commented Sep 20, 2017 •

edited

Loading

Sija commented Sep 21, 2017 •

edited

Loading

straight-shoota commented Sep 27, 2017 •

edited

Loading