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

Fix error message when parsing unknown JSON enum value #13728

Merged
merged 1 commit into from
Sep 12, 2023

Conversation

willhbr
Copy link
Contributor

@willhbr willhbr commented Aug 5, 2023

#string_value can be the value of the next token, which results in a confusing error message.

require "json"

enum Test
  Valid
  Other
end

struct Foo
  include JSON::Serializable
  @foo : Test
end

text = %({"foo": "value", "things": "Stuff"})

Foo.from_json(text)

On 1.9.2 this will raise an unhelpful error:

$ crystal run test.cr
Unhandled exception: Unknown enum Test value: "things" at line 1, column 26
  parsing Foo#foo at line 1, column 2 (JSON::SerializableError)
  from /usr/share/crystal/src/json/serialization.cr:182:7 in 'initialize:__pull_for_json_serializable'

Note that if it's not part of a larger JSON object, the message will be correct:

Test.from_json(%("value") # => Unhandled exception: Unknown enum Test value: "value"

`#string_value` can be the value of the next token, which results in a
confusing error message.
@willhbr
Copy link
Contributor Author

willhbr commented Sep 4, 2023

Is there anything else I should do here before it gets merged?

@straight-shoota
Copy link
Member

No, we're just waiting for a second approval from a Core Team member (see https://github.com/crystal-lang/crystal/blob/master/CONTRIBUTING.md#accepting-a-pull-request).

And please do not force push to a PR branch. That unnecessarily complicates reviewing due to lost history (see https://github.com/crystal-lang/crystal/blob/master/CONTRIBUTING.md#making-good-pull-requests).

@willhbr
Copy link
Contributor Author

willhbr commented Sep 5, 2023

Ah sorry, I'm used to doing endless rebases and hadn't seen that part of the contributing guide. Thanks for pointing that out.

@beta-ziliani beta-ziliani added this to the 1.10.0 milestone Sep 7, 2023
@straight-shoota straight-shoota merged commit 7b70718 into crystal-lang:master Sep 12, 2023
53 checks passed
Blacksmoke16 pushed a commit to Blacksmoke16/crystal that referenced this pull request Dec 11, 2023
This pull request was closed.
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.

4 participants