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 pattern, support reference constraints on primitives, and add number/integer constraints #264

Merged
merged 14 commits into from
Sep 16, 2024

Conversation

nolag
Copy link
Contributor

@nolag nolag commented Aug 16, 2024

No description provided.

@omissis
Copy link
Owner

omissis commented Sep 4, 2024

Hey @nolag , thanks for putting out this PR! Can you give me a heads up when it's ready (I see you're still pushing on it) so I can review it? :)

@nolag
Copy link
Contributor Author

nolag commented Sep 4, 2024

Hey @nolag , thanks for putting out this PR! Can you give me a heads up when it's ready (I see you're still pushing on it) so I can review it? :)

It's ready, I don't have more work planned. Each commit I made was meant to be independent, I figured I could open one PR and you could review it by commit if you wanted, but at this point, a squash probably makes more sense.

I can make a final commit tomorrow to resolve the merge conflict. I'm not sure why, but goland insisted on some of those changes and it wouldn't work without them.

@omissis
Copy link
Owner

omissis commented Sep 4, 2024

sweet! can you just fix the conflict in the go.mod? I'll look at it soon as I can!

@nolag
Copy link
Contributor Author

nolag commented Sep 5, 2024

sweet! can you just fix the conflict in the go.mod? I'll look at it soon as I can!

I realized I have one more commit locally that I need to clean up. I'll also fix the go mod issue. Still aiming for today.

@nolag nolag force-pushed the main branch 3 times, most recently from 6a308d7 to ec60cd3 Compare September 5, 2024 15:30
@nolag
Copy link
Contributor Author

nolag commented Sep 5, 2024

@omissis FYI, should be good to go soon, I can't reproduce the generation error locally though... I'll spend a bit of time to debug it, but if you can reproduce that would be helpful... I think it has to do with the casting floats to ints, I should probably use some epsilon or a better way like math.Round. I'll give that a try and ping you if I still need help

Sorry for all the force pushes at the end, I had some trouble getting the linter running locally for the tests (even with upstream/main).

I did find another bug that I didn't fix. Arrays don't apply rules for their elements. Ie: String's length in an array and now patterns and number bounds aren't checked.

I'll need to fix that eventually (or have it fixed) but it'll be a new PR at this point (it's not blocking our use case right now).

@nolag
Copy link
Contributor Author

nolag commented Sep 5, 2024

@omissis was able to get it consistently passing locally and on CI now. PR is good to go.

For context, the min int size is so that we can use binary data as a uint8, which we need. It's a flagged feature so it won't change any existing users :). It also allows more efficient checks when the number is bound by a set size.

Copy link

codecov bot commented Sep 5, 2024

Codecov Report

Attention: Patch coverage is 73.91304% with 138 lines in your changes missing coverage. Please review.

Project coverage is 75.77%. Comparing base (d963216) to head (733b746).
Report is 64 commits behind head on main.

Files with missing lines Patch % Lines
pkg/schemas/loaders.go 56.60% 17 Missing and 6 partials ⚠️
...ta/validation/exclusiveMaximum/exclusiveMaximum.go 45.45% 6 Missing and 6 partials ⚠️
...ta/validation/exclusiveMinimum/exclusiveMinimum.go 45.45% 6 Missing and 6 partials ⚠️
tests/data/validation/maximum/maximum.go 45.45% 6 Missing and 6 partials ⚠️
tests/data/validation/minimum/minimum.go 45.45% 6 Missing and 6 partials ⚠️
tests/data/validation/multipleOf/multipleOf.go 45.45% 6 Missing and 6 partials ⚠️
...validation/exclusiveMaximum/exclusiveMaximumOld.go 50.00% 5 Missing and 5 partials ⚠️
...validation/exclusiveMinimum/exclusiveMinimumOld.go 50.00% 5 Missing and 5 partials ⚠️
tests/data/validation/pattern/pattern.go 47.05% 5 Missing and 4 partials ⚠️
pkg/codegen/utils.go 86.20% 8 Missing ⚠️
... and 4 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #264      +/-   ##
==========================================
- Coverage   76.58%   75.77%   -0.81%     
==========================================
  Files          24       36      +12     
  Lines        1892     2089     +197     
==========================================
+ Hits         1449     1583     +134     
  Misses        354      354              
- Partials       89      152      +63     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Owner

@omissis omissis left a comment

Choose a reason for hiding this comment

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

Impressive PR! besides a couple nitpicks, I am just not sure about the exclusive minimum and maximum: I see they support two implementations, one with numeric exclusive min/max, and one with boolean + an extra value.
From what I see on jsonschema, the numeric one should be the correct one since draft-07, so I take you implemented both for BC reasons? in that case the boolean one should be the one that has the "Old" suffix, right? I'm referring to tests/data/validation/exclusiveMaximum/exclusiveMaximumOld.[json|go]

Thanks!

main.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
tests/data/validation/primitive_defs/primitive_defs.json Outdated Show resolved Hide resolved
@nolag
Copy link
Contributor Author

nolag commented Sep 16, 2024

Thanks for reviewing, and good catch on my mix-up with the exclusive min/max.

@nolag nolag requested a review from omissis September 16, 2024 14:52
@omissis omissis added this to the v0.17.0 milestone Sep 16, 2024
@omissis omissis merged commit bbba7f7 into omissis:main Sep 16, 2024
1 of 3 checks passed
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.

2 participants