krisprice / ipnet

IpNet, Ipv4Net, and Ipv6Net types and methods for Rust
Apache License 2.0
122 stars 26 forks source link

Add 'schemars' feature for deriving JSON schema #31

Closed smklein closed 2 years ago

smklein commented 3 years ago

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 commented 2 years ago

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

krisprice commented 2 years ago

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 commented 2 years ago

Sounds good!

ralpha commented 2 years ago

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 commented 2 years ago

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

ralpha commented 2 years ago

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 commented 2 years ago

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