-
Notifications
You must be signed in to change notification settings - Fork 19
Pydantic package organization episode 3: "System update complete" #403
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
Changes from all commits
d6ecb83
4a6c675
94f9438
4f1d2d3
c809b4f
999e4c0
ca1c80d
67ab3ff
babec58
dea80b4
aaf0646
dcb75ad
af29a35
51d7741
51f74dd
7ef9d3c
adb57c6
b6dda78
498d3f7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,13 +75,9 @@ | |
| }, | ||
| "additionalProperties": false, | ||
| "description": "Addresses are geographic points used for locating businesses and individuals. The\nrules, fields, and fieldnames of an address can vary extensively between locations.\nWe use a simplified schema to capture worldwide address points. This initial schema\nis largely based on the OpenAddresses (www.openaddresses.io) project.\n\nThe address schema allows up to 5 \"admin levels\". Rather than have field names that\napply across all countries, we provide an array called \"address_levels\" containing\nthe necessary administrative levels for an address.", | ||
| "patternProperties": { | ||
| "^ext_.*$": { | ||
| "description": "Additional top-level properties must be prefixed with `ext_`." | ||
| } | ||
| }, | ||
|
Comment on lines
-78
to
-82
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is one of the subtle 🐞 bugs addressed: {
"type": "Feature",
"geometry": {},
"properties": {},
"ext_foo": "bar",
"ext_bar": "baz"
}The extension properties would then get lost by the model validator that imports the GeoJSON into the Pydantic model class instance.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mmm, so it wasn't getting pivoted into
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was getting cloned into |
||
| "properties": { | ||
| "bbox": { | ||
| "description": "An optional bounding box for the feature", | ||
| "items": { | ||
| "type": "number" | ||
| }, | ||
|
|
@@ -129,9 +125,16 @@ | |
| }, | ||
| "properties": { | ||
| "additionalProperties": false, | ||
| "not": { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This fixes another subtle 🐞 bug: if these properties were allowed in under "properties", then you'd be open to this scenario in a GeoJSON input: {
"type": "Feature",
"id": "outer-id",
"bbox": [0, 0, 0, 0],
"geometry": {},
"properties": {
"id": "inner-id",
"bbox": [1, 1, 1, 1],
"geometry": "anything"
}
}The model validator would then copy everything from "properties" up to the root (overwriting the values in the root).
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The positive version means "it must be the case that it is there" and the negative version means "it must not be the case that it is there". So if you check out this mini-schema at https://www.jsonschemavalidator.net/... {
"type": "object",
"not": { "required": ["foo" ] },
"required": ["bar"]
}You find that this passes: Edit: Fixed an example that wrongly ended in a bracket
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TIL, wow. That's worth capturing (if it's not already) in the code that produces expressions like that. |
||
| "required": [ | ||
| "id", | ||
| "bbox", | ||
| "geometry" | ||
| ] | ||
| }, | ||
| "patternProperties": { | ||
| "^ext_.*$": { | ||
| "description": "Additional top-level properties must be prefixed with `ext_`." | ||
| "description": "Additional top-level properties are allowed if prefixed by `ext_`.\n\nThis feature is a on a deprecation path and will be removed once the schema is\nfully migrated to Pydantic." | ||
| } | ||
| }, | ||
| "properties": { | ||
|
|
@@ -221,19 +224,18 @@ | |
| "type", | ||
| "version" | ||
| ], | ||
| "type": "object", | ||
| "unevaluatedProperties": false | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Redundant - because
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you mean by "stricter"? IIRC,
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What I mean by stricter is that e.g. consider this JSON Schema: {
"type": "object",
"additionalProperties": false,
"properties": {
"foo": { "type": "integer" }
},
"patternProperties": {
"bar.*": { }
},
"allOf": [{
"properties": {
"baz": { }
}
}]
}The above will work for The reason this makes sense to me in the Pydantic context is that Pydantic models always have enumerated properties in the Edit: Added a missing not that was super confusing.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aie, that's confusing. Given the behavior you're showing, we might do well to avoid ever creating aggregators that introduce sub-schemas. |
||
| "type": "object" | ||
| }, | ||
| "type": { | ||
| "const": "Feature", | ||
| "type": "string" | ||
| } | ||
| }, | ||
| "required": [ | ||
| "type", | ||
| "id", | ||
| "geometry", | ||
| "properties", | ||
| "type" | ||
| "properties" | ||
| ], | ||
| "title": "address", | ||
| "type": "object" | ||
|
|
||
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.