Skip to content

Error improvements #69

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

Merged
merged 6 commits into from
Feb 6, 2025
Merged

Error improvements #69

merged 6 commits into from
Feb 6, 2025

Conversation

m-bock
Copy link
Contributor

@m-bock m-bock commented Jan 23, 2025

This is an alternative solution for PR #68 . It solves issue #66 .

The main idea is: The existing type Either JsonDecodeError a for decoders is extended by wrapping it into a custom Error:

data Err = UnmatchedCase | JErr JsonDecodeError

In this way an unmatched Case is treated as an error. In my view this makes the code a bit simpler, because we profit from short circuiting.

@wclr @garyb
Im curious about your opinions.

Test cases are added, too.

@m-bock m-bock mentioned this pull request Jan 23, 2025
@garyb
Copy link
Owner

garyb commented Jan 28, 2025

This is nice! I like @wclr's version too, but I like that this is a simpler set of changes for much the same result.

@wclr
Copy link

wclr commented Jan 28, 2025

I think the approach with a custom error type is more explicit and concise, though before merging I would like to modify my PR with this approach to point out some details that are not addressed in this one.

I also started to port the sum codec to codec-json.

@m-bock
Copy link
Contributor Author

m-bock commented Jan 30, 2025

@wclr Thanks for the feedback. Could you explain in words, what is not addressed here opposed to your PR?

@wclr
Copy link

wclr commented Feb 5, 2025

@m-bock looked closer.

  • Your errors now do not report a particular case in which error happend: (Named "Sample" (TypeMismatch "Expecting at least one element")) could report more contextful like (Named "Sample" (Named "case Baz" (TypeMismatch "Expecting at least one element" )))

Obviously not required because one may look at the data itself, but I think tag name context may be useful for quicker error debugging.

  • Some other things, I also tried to be more specific about the value index in which error occured (in tags with multiple values)

I added all the error cases to the tests, which I belive is good thing to have.

Also, in my PR I tried simplify the code of decoding/error logic to make it more unified and dry.

I've pushed the changes to my PR based on your approach with the custom error.

@m-bock
Copy link
Contributor Author

m-bock commented Feb 5, 2025

@wclr , I agree with @garyb that one of the main differences between the 'concurring' PR's is that this one here has a smaller diff. It's easier to review, also because it does only one thing.

Your PR contains a bit more, surely interesting and useful features.

Here's my suggestion: Let's merge this PR and you can create follow up PR's which add improvements step by step in an easy to review way.

@wclr
Copy link

wclr commented Feb 6, 2025

@m-bock No problem.

@garyb
Copy link
Owner

garyb commented Feb 6, 2025

👍 to that suggestion. Thanks both!

@garyb garyb merged commit 78b0171 into garyb:master Feb 6, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants