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 'schemars' feature for deriving JSON schema #31

Merged
merged 3 commits into from Mar 6, 2022

Conversation

smklein
Copy link
Contributor

@smklein smklein commented Jul 16, 2021

Context

schemars is a library which allows emission of JSON schema documents based purely on a type. It's a lot like serde (and has compatibility with serde) but allows emission of JSON schemas without a "value" - it operates purely based on type.

This is super handy for things that consume JSON schemas to define types - at Oxide (my employer), we use schemars to produce OpenAPI specs for our interfaces (and consequently, types that appear in our APIs derive Serialize, Deserialize, and JsonSchema).

Similar to the serde types, derive(JsonSchema) only works if all the contained types also implement JsonSchema.

Okay, what about this PR tho

This PR adds a new "feature" to ipnet, enabling the derivation of the JsonSchema type for IpNet types.
Similar to the existing serde feature, a new json feature provides a variant of this crate with the derives enabled.

For callers who want to use it: This enables usage of the IpNet types within other JsonSchema objects.
For callers who don't want it: It's behind a feature flag, so it remains completely unnoticeable (and has no impact on the compiled library).

@xfbs
Copy link

xfbs commented Dec 18, 2021

+1 on this from me. Although, you can probably get away with not using serde by deriving the JsonSchema yourself.

Cargo.toml Outdated
[features]
default = []
# Implements "serde::Serialize" and "serde::Deserialize".
serde = ["serde_crate"]
Copy link

Choose a reason for hiding this comment

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

Just by making the crate optional (optional = true, as already done) you can enable it using a feature flag of the crate name itself.

So then this line is not needed and serde feature can just be used immediately.
For more info: https://stackoverflow.com/a/39759592/2037998

This way some of the changes in src/ipnet_serde.rs are no longer needed to. That will make the PR a bit simpler.

Copy link

Choose a reason for hiding this comment

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

I've just pushed something, take a look. Let me know if that is good

Copy link

Choose a reason for hiding this comment

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

I don't see the changes anywhere. Where did you push them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went ahead and collapsed this, avoiding the renaming.

cargo build/test --features serde and cargo build/test --features json are both happy.

@krisprice
Copy link
Owner

Thanks @smklein, @ralpha, and @xfbs. Sorry for the long delay. Per the conversation. Shouldn't we be able to just have schemars as a new optional dependency here, then without changing anything else we can add the #[cfg_attr(feature = "json", derive(schemars::JsonSchema))] lines in ipnet.rs? Does that work @smklein ? I'm not sure if the resulting output is satisfactory or if you want it customized a little. If that works and you want to make those changes I'll go ahead and merge this.

@xfbs
Copy link

xfbs commented Feb 15, 2022

Sounds good!

@ralpha
Copy link

ralpha commented Feb 15, 2022

Shouldn't we be able to just have schemars as a new optional dependency here, then without changing anything else we can add the #[cfg_attr(feature = "json", derive(schemars::JsonSchema))] lines in ipnet.rs?

Yes that would work. But the change in: src/ipnet_serde.rs:67

- match try!(data.variant()) {
+ match data.variant()? {

I would keep too. The try! macro has been deprecated for some time now: https://doc.rust-lang.org/std/macro.try.html

@smklein
Copy link
Contributor Author

smklein commented Feb 16, 2022

Avoided making serde a separate high-level feature (plus the crate renaming shenanigans) - PTAL!

Comment on lines 81 to 82
#[cfg(feature = "serde")]
extern crate serde_crate;
extern crate serde;
Copy link

Choose a reason for hiding this comment

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

This code can be removed. This is no longer needed in Rust with edition = "2018", so the edition might have to be set. But can also happen in other PR.

@ralpha
Copy link

ralpha commented Feb 16, 2022

If this compiles and works it looks all good to me.
I guess #[macro_use] for serde was never used?

@krisprice I might suggest added an edition to the crate, best to use edition = "2018" unless you know that nobody with a Rust version of less then 1.56.0 is using it you can go for 2021. But might be a bit soon for a lower level lib. (can be done separate from this PR)

Other then those remarks it looks all good.

@krisprice krisprice merged commit dacd04a into krisprice:master Mar 6, 2022
@krisprice krisprice mentioned this pull request Mar 6, 2022
@krisprice
Copy link
Owner

Thanks again @smklein, @ralpha, and @xfbs : now merged and published.

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.

None yet

4 participants