-
Notifications
You must be signed in to change notification settings - Fork 204
Add formatting and validation for free-form fields #272
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
base: main
Are you sure you want to change the base?
Conversation
269b060
to
305ff32
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
1e7e79e
to
4b726f2
Compare
4b726f2
to
7e9f2be
Compare
@tadasant please review. |
[EDIT: see comment below...] I think where possible, I'd want to push this into the JSON schema spec as much as possible - this means:
Of course there will be exceptions to this where we really can't do it with JSON schema. But a lot of this I think could be replicated with e.g. length or regex requirements on the JSON schema etc. |
Hmm actually just read up on #97, and that maybe we only want this to be on the official registry API spec (rather than the generic registry API spec). Based on @.tadasant's comment:
I guess in that case the approach you've gone for here is fine - I'll leave it up to @tadasant to decide / review :) |
@domdomegg - isn't it preferred to still have the validation in the schema even if it's just for registry-server.json? |
@rdimitrov possibly true? I guess then it'd be harder to take the official-registry-api-schema.json and turn it into the registry-api-schema.json. But maybe this is fine and/or we shouldn't maintain them this way. |
@domdomegg / @tadasant can we conclude on the discussion whether we want to add the validation in the spec and let huma handle it or we should go ahead with the code based validation? It's fine if we need more time to think it through. |
Sorry I'm well behind on reviews and discussions and trying to focus where I'm blocking @domdomegg - trust his call on this one if he wants to jump ahead and get this merged (otherwise I will get to this eventually, just will be slow) This is correct:
The intent is for this formatting/validation to only apply to the official registry. Most importantly at publication-time (we don't want people publishing data that is noncomformant to these expectations) I haven't yet formed an opinion on how that cascades to the implementation here |
No worries @tadasant, take your time. @domdomegg you can also review if you want to. |
Okay I've had a think about this, and my conclusion is that I think it doesn't matter too much either way 😅 I have a weak preference towards doing it as schema annotations in huma, rather than custom code validations. I think this documents it better for users of the API, and usually is less verbose/puts all the schema info in one place. I'd be happy to accept this PR, or one doing it with schema annotations, once the code is cleaned up a little (e.g. linting and tests pass). @Avish34 are you good to take up implementing it one of these ways, then pinging me once you have CI passing? |
The changes are intended to create validators for all the free-form fields. Please find the details of the validation of different field below.
TODOs
Motivation and Context
The motivation was to make the code clean and add base code for more validator for different objects like Repository etc.
#97
How Has This Been Tested?
It's tested via UTs. Added UT's for all the validations.
Breaking Changes
Yes
Types of changes
Checklist
Additional context