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

support const generics #205

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cameron1024
Copy link

I'd like to support const generic arrays, and it didn't seem to hard to implement, so I made a quick PR - of course, I'm not 100% familiar with your design goals, so if this isn't how you'd like things done, let me know and I'd be happy to change it up 😁

This PR adds JsonSchema impls for const generic arrays, allowing both arrays with size greater than 32, as well as arrays of any size (for reference, I ran into this while trying to implement JsonSchema on a type in my crate that wraps a [u8; N]).

There's a couple of downsides:

  • this is a breaking change, since the existing impl made special exceptions for 0-length arrays, allowing schemas to generated even for 0-length arrays of types that don't implement JsonSchema.. AFAIK, doing this is impossible in the presence of a const-generic impl without specialization. The compromise was to export a newtype wrapper: EmptyArray<T>, which provides that impl. Of course, this means that downstream users who relied on [RandomStruct; 0]: JsonSchema will now need to replace [RandomStruct; 0] with EmptyArray<RandomStruct>.
  • It also bumps the MSRV to 1.51 - the minimum version which supports const generics. I couldn't find an MSRV policy anywhere, so this didn't seem like a huge concern - again, happy to be corrected on this.

I'm not an expert w.r.t. json schema in general, so I guess it seems weird to me to want a schema that says: "this field expects a 0-length array of an unrepresentable type". Is this common in the wild? What's the use-case?

My first thought would actually be to not have the EmptyArray type at all, since that seems so "out there", but given that there's already specific tests in place checking that behaviour exists, I thought it best to check before just removing support.

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.

1 participant