-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
[BUGFIX] Fix typing on mostly
and value_set
fields
#10571
Conversation
✅ Deploy Preview for niobium-lead-7998 canceled.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## develop #10571 +/- ##
===========================================
- Coverage 80.21% 80.18% -0.03%
===========================================
Files 461 461
Lines 40002 39946 -56
===========================================
- Hits 32088 32032 -56
Misses 7914 7914
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -81,7 +81,7 @@ | |||
}, | |||
"value_set": { | |||
"title": "Value Set", | |||
"description": "A list of potential values to match.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the only instance where we had a different description for the value set. I think it makes sense to make it the same as the others.
@@ -80,12 +80,12 @@ | |||
}, | |||
"mostly": { | |||
"title": "Mostly", | |||
"default": 1.0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lotta noise on these. It's functionally the same, just a different ordering for mostly
.
mostly
and value_set
fields
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good - thanks for fixing this
Context
We were previously using
Annotated
incorrectly. The correct way to use it isAnnotated[T, x]
, whereT
is the base case, andx
is the metadata. We had those type args backward. Pydantic uses the first argument for validation, and schema generation, so schemas were generated as expected, but mypy was upset because e.g.mostly=1
does not satisfy the type hint that mostly must be an instance of_Mostly
. So we had to change a bunch of stuffAbout our validation vs schema.
Our validation was fairly lax, so I kept it that way in this PR. e.g.
multiple_of
was not enforced, but was just there for the schema. Pydantic actually has support out of the box for most of what we were doing, includingmultiple_of
, but since we didn't enforce this before, I didn't want to start enforcing it now.The problem
From experimenting, it looks like constraints on nested annotations are not supported in schema generation. See #10577 for how I'd like to have written the solution, and how it fails in CI.
Implementation details
Annotated
type argspydantic.Field
allowing for arbitrary fields to be passed in AND theschema_extra
override on the base Expectation class. Basically duringschema_extra
, we pop out a custom field,schema_overrides
, and write the contents of it to whatever property it was on.I just copy/pasted what was in the snapshots of our expectation schema json for
value-set
. Formostly
, I used out of the box functionality where I could, and only passed themultiple_of
inschema_overrides
. The change in how these fields are added to the schema results in a bunch of noise in the PR due to ordering changes.invoke lint
(usesruff format
+ruff check
)For more information about contributing, see Contribute.
After you submit your PR, keep the page open and monitor the statuses of the various checks made by our continuous integration process at the bottom of the page. Please fix any issues that come up and reach out on Slack if you need help. Thanks for contributing!