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.

Sign up for GitHub

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

Jump to bottom

Fix YAML scalar type validation error message #13771

Merged
merged 10 commits into from
Sep 22, 2023
Merged

Fix YAML scalar type validation error message #13771

merged 10 commits into from
Sep 22, 2023

Conversation

MistressRemilia
Copy link
Contributor

This should fix #13770

@Blacksmoke16
Copy link
Member

Some tests would be good. Can maybe use the JSON equivalent of this as a guide?

#8698

@MistressRemilia
Copy link
Contributor Author

That was sill of me, I forgot the specs. I'll add those this weekend.

@MistressRemilia
Copy link
Contributor Author

It looks like standard library specs already have a few tests to ensure that fields that expect an integer smaller than Int64 are parsed into the correct type ("allows small types of integer"). What's missing is the checks to ensure that YAML parsing raises a correct exception when the parsed value does not fit within the expected, type (the original issue that prompted this PR). I think the two specs I added should cover the missing cases in this case. If they need to be adjusted, please let me know and I'll be happy to adjust them ^_^

straight-shoota
straight-shoota approved these changes Aug 29, 2023
View reviewed changes
Copy link
Member

@straight-shoota straight-shoota left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, I just a minor suggestion for improvement ☝️

MistressRemilia and others added 5 commits August 29, 2023 02:34
@MistressRemilia @straight-shoota
Update src/yaml/from_yaml.cr
584d84f 
Co-authored-by: Johannes Müller <straightshoota@gmail.com>
@MistressRemilia @straight-shoota
Update src/yaml/from_yaml.cr
19deaa3 
Co-authored-by: Johannes Müller <straightshoota@gmail.com>
fix typo
81df9fa
Merge branch 'master' of github.com:MistressRemilia/crystal
c2d59aa
Fix value
2ffaf60
@straight-shoota straight-shoota added this to the 1.10.0 milestone Sep 22, 2023
@straight-shoota straight-shoota changed the title Ensure the parsed value will fit in the requested numeric when deserializing YAML Fix YAML scalar type validation error message Sep 22, 2023
@straight-shoota straight-shoota merged commit c09562a into crystal-lang:master Sep 22, 2023
53 checks passed
Blacksmoke16 pushed a commit to Blacksmoke16/crystal that referenced this pull request Dec 11, 2023
@MistressRemilia @straight-shoota @Blacksmoke16
Fix YAML scalar type validation error message (crystal-lang#13771)
04ba25b 
Co-authored-by: Johannes Müller <straightshoota@gmail.com>
Co-authored-by: yukiraven <alexa@partition36.com>
Co-authored-by: MistressRemilia <4798372-RemiliaScarlet@users.noreply.gitlab.com>
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@straight-shoota straight-shoota straight-shoota approved these changes

@HertzDevil HertzDevil HertzDevil approved these changes

Assignees
No one assigned
Labels
kind:bug topic:stdlib:serialization
Projects
None yet
Milestone
1.10.0
Development

Successfully merging this pull request may close these issues.

Integer types are not type checked as expected when deserializing YAML
4 participants
@MistressRemilia @Blacksmoke16 @straight-shoota @HertzDevil