Skip to content

Add serde support to AST structs and enums #183

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
panarch opened this issue Jun 5, 2020 · 6 comments
Closed

Add serde support to AST structs and enums #183

panarch opened this issue Jun 5, 2020 · 6 comments

Comments

@panarch
Copy link
Contributor

panarch commented Jun 5, 2020

Hello, I found that serde is not applied in this project, so there's no easy way to serialize/deserialize AST nodes.
serde is often quite useful for some cases; considering to parse query and store the result in somewhere, or parsing remotely and send to database over network... etc.

It looks good to add serde as optional like bigdecimal.
then something similar with this below will be added.

# Cargo.toml
serde = { version = "1.0", features = ["derive"], optional = true }

And for every ast/ codes,

#[cfg(feature = "serde")]
use serde::{Serialize, Deserialize};

/// Unary operators
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
pub enum UnaryOperator {
    Plus,
    Minus,
    Not,
}

How do you think of this? If it is ok, then I'd like to work on this issue.

@nickolay
Copy link
Contributor

nickolay commented Jun 7, 2020

I agree. I actually add this by hand to my local copy every now and then using two global replacements:

  • Search for #[derive(Debug, Clone, PartialEq, Eq, Hash)] to append Serialize/Deserialize
  • Search for use std::fmt; and prepend use serde::{Serialize, Deserialize};

...but if I'm not alone, it's probably a good idea to include it in the crate, via an optional feature, as you suggest.

@panarch
Copy link
Contributor Author

panarch commented Jun 7, 2020

@nickolay Thanks, it's so good to see that I'm not the only one who needs serde.
I also worried about that someone may think serde is totally not necessary feature to have.
So... that was why I suggested to add as an optional feature, cfg is really cool.

I'm currently migrating a parser of my existing sql db project from nom-sql to sqlparser-rs, this one.
In nom-sql, it supports serde so I used a lot and my project is quite depending on serde.
I'm using serde to store table schema, by using data types from provided by parser.

And... because I should use serde, so I actually already finished applying it on my local copy, like you.
panarch@9335c34
Here you can see, I forked and it is in my serde branch.

Hope to get positive feedback from others.

@Dandandan
Copy link
Contributor

Serde support could be quite useful. I agree as an optional feature would be the best, to avoid longer compile times.

@panarch
Copy link
Contributor Author

panarch commented Jun 9, 2020

Due to https://github.com/andygrove/sqlparser-rs/pull/189
nickolay@5ba4a55 already added serde so I'll just leave my local branch without requesting PR. 👌

@nickolay
Copy link
Contributor

nickolay commented Jun 9, 2020

That's just an instance of adding this by hand to my local copy, not intended to be merged (it's not optional, for one). Please feel free to PR your branch.

@panarch
Copy link
Contributor Author

panarch commented Jun 10, 2020

Ok, then I'll make PR, thanks :)

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

No branches or pull requests

3 participants