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

Upgrade to jsonschema 4.18.0 #123

Closed
wants to merge 6 commits into from
Closed

Upgrade to jsonschema 4.18.0 #123

wants to merge 6 commits into from

Conversation

jpmckinney
Copy link
Contributor

@jpmckinney jpmckinney commented Jul 7, 2023

Closes #112
Closes #130

As described in the documentation linked from #112 , the new version of jsonschema separates resolution from caching.

RefResolver had no handlers by default. lib-cove had copied these lines.

This also fixes add_is_codelist() being unreachable from one branch of the if-statement, a bug introduced in 011a2bd

The upgraded causes test_property_that_is_not_json_schema_doesnt_raise_exception to now raises an error. I'm not sure why this scenario shouldn't raise an error.

jsonschema 4.18 drops support for Python 3.6 and 3.7.

Includes #118

@jpmckinney
Copy link
Contributor Author

Test failure is from BODS being included, again.

@jpmckinney
Copy link
Contributor Author

I made #128 and #129 now as easier-to-merge alternatives, which give lib-cove-ocds the flexibility it needs to (1) use the latest jsonschema and (2) not see deprecation warnings.

@odscjames
Copy link
Collaborator

I made #128 and #129 now as easier-to-merge alternatives, which give lib-cove-ocds the flexibility it needs to (1) use the latest jsonschema and (2) not see deprecation warnings.

As those 2 are now merged, is this important to do?

@jpmckinney
Copy link
Contributor Author

jsonschema will continue to release new versions, so it makes sense to keep up with its development.

Also, this prevents a scenario where you’ll be tempted to set a maximum version on jsonschema when some future version causes an error.

@jpmckinney
Copy link
Contributor Author

Changes are quite small when you ignore whitespace: https://github.com/OpenDataServices/lib-cove/pull/123/files?w=1

The main change is the minimum version.

@odscjames
Copy link
Collaborator

This PR removes

Allow SchemaJsonMixin classes to define a validator method, that accepts lib-cove's JSON Schema draft 4 validator class and its format checker, and returns a validator instance. #128

That is on main but is not released yet - replaces it with specifying a "registry" value instead. Is this ok?

@jpmckinney
Copy link
Contributor Author

I would like validator to be preserved as a method. In lib-cove-ocds, I wrap it in an lru_cache, which has a big impact when running the validator millions of times.

I don't think this PR removes validator. It just branched from main before validator was extracted to a method.

@jpmckinney
Copy link
Contributor Author

@odscjames I fixed the merge conflicts, in case that helps.

@jpmckinney
Copy link
Contributor Author

jpmckinney commented Dec 6, 2023

@odscjames I've added a new block to cove-oc4ids to display additional_closed_codelist_values and I have a branch ready for lib-cove-oc4ids where tests pass under the new version of lib-cove.

So, please don't let oc4ids or ocds integration tests be a blocker – I'll take responsibility for those.

@jpmckinney
Copy link
Contributor Author

This PR's diff is easier to read with whitespace ignored: https://github.com/OpenDataServices/lib-cove/pull/123/files?w=1

odscjames added a commit that referenced this pull request Dec 8, 2023
odscjames pushed a commit that referenced this pull request Dec 8, 2023
odscjames pushed a commit that referenced this pull request Dec 8, 2023
@jpmckinney
Copy link
Contributor Author

Done in #142

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants