Skip to content

Add code example to see how to use serde #197

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 4 commits into from
Jun 12, 2020
Merged

Add code example to see how to use serde #197

merged 4 commits into from
Jun 12, 2020

Conversation

panarch
Copy link
Contributor

@panarch panarch commented Jun 10, 2020

Related to PR: https://github.com/andygrove/sqlparser-rs/pull/196

Because serde is an optional dependency, if I modify existing example codes, then it may make other new comers feel bothersome.
So I added new code example named examples/serde.rs.
@nickolay How do you think of this?

@coveralls
Copy link

coveralls commented Jun 10, 2020

Pull Request Test Coverage Report for Build 132333060

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 91.943%

Totals Coverage Status
Change from base Build 130895395: 0.0%
Covered Lines: 4051
Relevant Lines: 4406

💛 - Coveralls

@nickolay
Copy link
Contributor

My suggestion was to include it as an option to an existing example, like this:

https://github.com/andygrove/sqlparser-rs/pull/189/files#diff-6d3ae06537fccb04d134faab9e782c45R61

So that we could recommend something like cargo run --features serde --example cli test.sql in README for people who don't care about Rust and want to get the parse results in a portable format. I think JSON serialization would work better than ron for that.

What do you mean by bothersome?

@panarch
Copy link
Contributor Author

panarch commented Jun 10, 2020

I think now I got the point.

I meant bothersome, it just comes from misunderstanding of the purpose of cli.rs file.
I simply thought examples/ codes are only for helping users to understand code easily, so adding something more feature is not necessary, that was I thought. But...
"README for people who don't care about Rust and want to get the parse results in a portable format."
I didn't think about this kind of use cases, and it's cool. Then including serde in cli.rs is worth enough to do.

And I also didn't know cfg use patterns you mentioned.
I thought it's not possible to clearly divide between using cfg and not using it.
I didn't aware of..

if cfg!(not(feature = "cst")) { ... }

#[cfg(feature = "cst")]
{
//...
}

Those kind of patterns, that's really cool. Thanks to let me know!

Ok, then it makes sense to apply serde in cli.rs.
And at this point, I also agree serde_json is way better than ron.

I'll update cli.rs and remove serde.rs and also update README by focusing on how to use cli command to see the result in simple way.

@nickolay
Copy link
Contributor

I forgot that doing that would build serde unconditionally in dev mode (as cargo doesn't allow optional dev-dependencies). I try to minimize the amount of stuff we build by default to keep the build times down. Which means we'd have to move the cli example to a separate crate like we did for the benchmarks in https://github.com/andygrove/sqlparser-rs/pull/190

Or move serde_json to [dependencies] just to make it optional. I did the latter in the follow-up push: andygrove@3a05b32 - what do you think of this?

@panarch
Copy link
Contributor Author

panarch commented Jun 12, 2020

Wow, that both two options sound very reasonable, it's quite tough to pick only one.
I didn't think about whether adding optional to dev-dependencies are possible or not.
If that is possible, then that could be simple and clean solution for now. But it does not work so...

Ok, than how about taking your adding serde_example feature option?

If cli has more roles and examples/ has more files to see, than it would be worth enough to split into different crates.
But current cli usage example is simple enough to embed in main README.md and current main Cargo.toml is also simple for now. So although it requires some comments like this andygrove@3a05b32#diff-80398c5faae3c069e4e6aa2ed11b28c0R29-R31, I think it's clear enough to see what's going on.

By the way, https://github.com/andygrove/sqlparser-rs/pull/190 was also so good idea to take, thanks for sharing, it was so fun to see what was going on there.

@nickolay nickolay merged commit a0f076a into apache:master Jun 12, 2020
@nickolay
Copy link
Contributor

If cli has more roles and examples/ has more files to see, than it would be worth enough to split into different crates.

This makes sense. So I merged the single-crate variant for now. Thanks for your help!

@panarch
Copy link
Contributor Author

panarch commented Jun 13, 2020

andygrove@a0f076a#diff-6d3ae06537fccb04d134faab9e782c45R67-R70

if cfg!(feature = "json_example") {
    #[cfg(feature = "json_example")]
    {
        ...
    }
...

by the way, cool, this looks better than put #[cfg(..)] in else statement. 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

Successfully merging this pull request may close these issues.

3 participants