Skip to content

Implement Serialize and Deserialize on ParseError #386

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

Closed
wants to merge 1 commit into from

Conversation

KiChjang
Copy link
Contributor

@KiChjang KiChjang commented Aug 6, 2017

This change is Reviewable

@KiChjang
Copy link
Contributor Author

r? @SimonSapin

@Hoverbear Hoverbear changed the title Implement Serialize and Deserialize on De<ParseError> Implement Serialize and Deserialize on ParseError Aug 29, 2017
@Hoverbear
Copy link

I updated the ticket name to reflect that it does both Ser and De.

/// Deserializes this ParseError from a `serde` stream.
impl<'de> Deserialize<'de> for De<ParseError> {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error> where D: Deserializer<'de> {
enum Variant {

Choose a reason for hiding this comment

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

I'm a bit unhappy with the level of duplication here. Is there maybe some way we can reduce the amount of times we list all the variants? In the interest of maintainability it would be great to only need to change the code in one (or two) spots when we add a variant. At the moment it seems like we need to do it in at least 6 places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The duplication here is mostly because of my own lack of knowledge of how serde works. I have no idea how the implementation body of ParseErrorVisitor would look otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I'm under the impression that url_serde is only a temporary fix until we upgrade to serde 1.0 on master, so I guess this is fine for now?

@SimonSapin SimonSapin mentioned this pull request Oct 17, 2018
42 tasks
@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #517) made this pull request unmergeable. Please resolve the merge conflicts.

@SimonSapin
Copy link
Member

master now depends on serde 1.x, so it should be possible to use serde_derive which would be much simpler than this PR. Feel free to open another PR doing that.

@SimonSapin SimonSapin closed this Jul 17, 2019
@KiChjang KiChjang deleted the parse-error-serde branch July 17, 2019 21:30
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.

4 participants