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

Add Serde Support For AST #77

Closed
wants to merge 7 commits into from
Closed

Conversation

anshap1719
Copy link

No description provided.

@LegNeato
Copy link
Member

LegNeato commented Dec 6, 2023

Should this support deserializing as well?

@anshap1719
Copy link
Author

@LegNeato I think that would be ideal, yes. Sorry I didn't realize that I didn't add it in. WIll make the change.

@anshap1719
Copy link
Author

@LegNeato I now remember why I didn't add it initially. It's because of having to deal with lifetimes and I didn't want to bother with that at the time. I will eventually (hopefully soon) add it in, but just wanted to give you an update here.

@LegNeato
Copy link
Member

LegNeato commented Jan 2, 2024

@tyranron are you ok with this landing without deserialize or do you want to wait? (I haven't reviewed yet FYI)

@tyranron
Copy link
Member

tyranron commented Jan 5, 2024

@LegNeato I think we need @tailhook's decision/opinion here as a main maintainer of this crate. I haven't actually dived into graphql-parser's code yet.

@LegNeato
Copy link
Member

LegNeato commented Jan 5, 2024

Ah yeah, I was in the wrong project 🤣

Copy link
Member

@LegNeato LegNeato left a comment

Choose a reason for hiding this comment

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

This looks fairly mechanical and good. Can you add a test (this could just be calling a no-op function that has the Serialize bound) and add using / not using this flag to CI so we don't accidentally break it?

@sodiumjoe
Copy link

👋 I need both serialization and deserialization for a project I'm working on. I am willing to try to finish this PR by adding a test and also adding deserialization, but the second one might be a bit beyond my abilities.

I checked out the wip commit before the reverts of the deserialization, but haven't been able to fix the lifetime issues. If someone is willing and available to provide some guidance, I'm still happy to try to make progress on this.

The lifetime issues are pretty tricky as they seem to come from the derive macro implementation.

On this line, the + Serialize + Deserialize<'a> seem unnecessary, as the implementer of the Text trait should impl them if required.

When I try removing those, I get fewer (10) compiler errors, that look like this

help: `'de` and `'a` must be the same: replace one with the other

error: lifetime may not live long enough
  --> src/schema/ast.rs:43:25
   |
38 | #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
   |                                                 ----------- lifetime `'de` defined here
39 | pub enum Definition<'a, T: Text<'a>> {
   |                     -- lifetime `'a` defined here
...
43 |     DirectiveDefinition(DirectiveDefinition<'a, T>),
   |                         ^^^^^^^^^^^^^^^^^^^ requires that `'de` must outlive `'a`
   |
   = help: consider adding the following bound: `'de: 'a`

I tried using serde(borrow = "'a") as described here, but that doesn't seem to work.

Any help appreciated!

@sodiumjoe
Copy link

I got some help from a colleague: #80

@anshap1719
Copy link
Author

Thanks @sodiumjoe, I'll close this PR in favour of yours.

@anshap1719 anshap1719 closed this Jun 20, 2024
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