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

Centralise error positioning + cover custom de errors #356

Merged
merged 5 commits into from
Feb 19, 2022

Conversation

juntyr
Copy link
Member

@juntyr juntyr commented Dec 30, 2021

Fixes #203 and supersedes #209. Also includes some small minor refactoring of the ErrorCode names.

In particular, the Deserializer now uses ErrorCode as its error type, no longer having to pretend to produce error positions in all cases (which as #209 showed were missing in all serde-internal and user-generated cases, and can only be added during deserialization to the former). Instead, the exposed helper functions from_str etc. now add the positional information afterwards, which cleans up the code and gives us positions for custom errors in serde-code and user-code for free (though the position still only points at the character after the error or directly at it).

  • I've included my change in CHANGELOG.md

@juntyr juntyr marked this pull request as ready for review December 30, 2021 10:02
@juntyr
Copy link
Member Author

juntyr commented Dec 30, 2021

?r @torkleyy @kvark

@juntyr
Copy link
Member Author

juntyr commented Dec 30, 2021

There is also the question whether ron should split the error type into two, one for ser and one for de, or keep the combined one (like serde_json). What are your thoughts? For ser it seems like we only generate Io and Message errors that are never positioned anyways (unless we wanted to arbitrarily track a fake line and column somehow).

@juntyr
Copy link
Member Author

juntyr commented Jan 26, 2022

?r @torkleyy Any thoughts?

@kvark
Copy link
Collaborator

kvark commented Feb 18, 2022

I'm very sorry that neither me or torkley are responsive!
If you are confident with the move, please feel free to commit without reviews if N days passed without any feedback (as long as this deadline is visible). For this PR, I'll review it now.

src/de/id.rs Outdated Show resolved Hide resolved
@kvark kvark merged commit e542eff into ron-rs:master Feb 19, 2022
@juntyr juntyr deleted the 203-error-positions branch February 19, 2022 22:03
@torkleyy torkleyy mentioned this pull request Jun 6, 2022
2 tasks
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.

Some errors are missing line number information.
2 participants