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

Add YAML::Nodes::Node#kind #10432

Merged

Conversation

straight-shoota
Copy link
Member

@straight-shoota straight-shoota commented Feb 20, 2021

This method is already included in #10431 but this is a proper introduction and wide-spread adaptation.

Node#typeNode#kind returns a string represntation of the node type (document, scalar, sequence, mapping, alias) and makes error message nicer. Now they say Expected scalar, not sequence instead of Expected scalar, not YAML::Nodes::Sequence. There's no need for the full Crystal path name.

EDIT: The method name was changed from Node#type to Node#kind.

@straight-shoota
Copy link
Member Author

It's a bit disappointing that only a single spec needed to be updated. This probably means that edge cases in YAML parsing are not tested much (at least not the expected error messages).

Copy link
Member

@bcardiff bcardiff left a comment

Choose a reason for hiding this comment

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

I usually prefer to use kind, data_type or other word that is neither class nor type. But if it's yaml jargon it could be fine.

@straight-shoota
Copy link
Member Author

I'm actually not sure this is a proper term for yaml. kind is probably good.

@bcardiff
Copy link
Member

In the specs the term "data type" is used, and sometimes "node kind". Node kind is scalar and can hold atomic data types.

@straight-shoota
Copy link
Member Author

The parser uses EventKind enum which is a bit more granular, but basically the same concept. I'll change it to kind. 👍

@straight-shoota straight-shoota changed the title Add YAML::Nodes::Node#type Add YAML::Nodes::Node#kind Feb 21, 2021
@bcardiff bcardiff added this to the 1.0.0 milestone Feb 21, 2021
@bcardiff bcardiff merged commit 248dd97 into crystal-lang:master Feb 21, 2021
@straight-shoota straight-shoota deleted the feature/yaml-parse-exception branch February 21, 2021 18:12
straight-shoota added a commit to straight-shoota/crystal that referenced this pull request Feb 22, 2021
asterite pushed a commit that referenced this pull request Feb 23, 2021
* Serialize Enum to underscored String by default

* Quote enums that serialise to YAML keywords

* Account for libyaml version

Co-authored by [email protected]

* Serialize Flags enums to and from json as an array

* Serialize Flags enums to and from yaml as an array

* Sum parsed flag enum members before calling `from_value`

* Add JSON::PullParser#raise

* Add YAML::Nodes::Node#type

* Improve specs and more strict implementation

* Remove unnecessary token checks in JSON impl.

* crystal tool format

* Refactor to NumberConverter and simplify deserialization code

* [CI] Print libyaml version

* Fix spec libyaml compat

* Add documentation

* Update specs after #10432

Co-authored-by: Caspian Baska <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants