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

Unpin jsonschema, fix default validation #1203

Merged
merged 5 commits into from
Oct 11, 2022

Conversation

braingram
Copy link
Contributor

@braingram braingram commented Oct 3, 2022

Add custom default validator to 'applicable_validators' argument to jsonschema.validators.create

Tested locally with jsonschema 4.16

This is work towards fixing #1172

One test is still failing locally (a doctest in docs/asdf/config.rst) but I wanted to trigger the CI and discuss the changes so far.

closes #1177

@WilliamJamieson WilliamJamieson marked this pull request as draft October 3, 2022 22:49
@WilliamJamieson
Copy link
Contributor

Unfortunately, we cannot unpin jsonschema with these changes as the DeprecationWarning raised by the way we use the Validator in other places does not allow us to validate schemas using our pytest plugin (we do not accept any warnings while testing schemas). The changes here need to be combined with something that solves the `DeprecationWarning such as PR #1185.

@WilliamJamieson
Copy link
Contributor

I opened PR: braingram#1 on your branch in your fix for the config.rst failure you are seeing. Feel free to merge it to fix your bug.

@WilliamJamieson
Copy link
Contributor

Also your history is messed up. As a general rule, we do not accept PRs with merge commits inside them like 48527ec (note that merge commits of a PRs onto your branch are fine). This is so we can have a clean history.

This is typically caused by trying to use the "Update branch" button on github. Instead, you should do a git rebase locally onto master and then force push the result. (Note that similar issues maybe caused by using the "Sync fork" option on github, though I have not checked).

@braingram braingram force-pushed the feature/jsonschema_4.10 branch 2 times, most recently from d444815 to 5ab2193 Compare October 4, 2022 12:23
@braingram
Copy link
Contributor Author

Thanks! I rebased and accepted your PR braingram#1.

Thanks also for pointing out #1185 Am I understanding the CI logs right in thinking the errors are in tests for asdf-standard? I have not run those locally and will do so.

Copy link
Contributor

@WilliamJamieson WilliamJamieson left a comment

Choose a reason for hiding this comment

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

I think we should limit the scope away from releasing the jsonschema pin and focus on fixing jsonschema as a dev dependency instead.

pyproject.toml Outdated Show resolved Hide resolved
CHANGES.rst Outdated Show resolved Hide resolved
@braingram
Copy link
Contributor Author

Ran pytest locally with jsonschema==4.0.1. All asdf tests passed.

@WilliamJamieson
Copy link
Contributor

It is unfortunate that the devdeps jobs are not allowing an install with the development dependency. It will take more effort to resolve this...

@WilliamJamieson
Copy link
Contributor

A simple approach is, we merge #1185 with the tests this fixes marked as xfail. Then rebase this PR on the result and then remove the xfail marks.

braingram added a commit to braingram/asdf that referenced this pull request Oct 5, 2022
WilliamJamieson added a commit to WilliamJamieson/asdf that referenced this pull request Oct 11, 2022
@WilliamJamieson
Copy link
Contributor

@braingram can you rebase this now that #1185 is merged and remove the two xfail in the schema tests?

braingram and others added 4 commits October 11, 2022 15:59
Add custom default validator to 'applicable_validators' argument
to jsonschema.validators.create

Tested locally with jsonschema 4.16

update changelog

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci
Co-authored-by: William Jamieson <[email protected]>
requirements-dev.txt Outdated Show resolved Hide resolved
@WilliamJamieson WilliamJamieson marked this pull request as ready for review October 11, 2022 21:27
@WilliamJamieson WilliamJamieson merged commit 8632535 into asdf-format:master Oct 11, 2022
bmwiedemann pushed a commit to bmwiedemann/openSUSE that referenced this pull request Oct 22, 2022
https://build.opensuse.org/request/show/1030520
by user bnavigator + dimstar_suse
- Update to 2.13.0
  * The ASDF Standard is at v1.6.0
  * Add ability to pull information from schema about asdf file
    data, using ~asdf.AsdfFile.schema_info method. [#1167]
- Release 2.12.1
  * Overhaul of the ASDF documentation to make it more consistent
    and readable. [#1142, #1152]
  * Update deprecated instances of abstractproperty to
    abstractmethod [#1148]
  * Move build configuration into pyproject.toml [#1149, #1155]
  * Pin jsonschema to below 4.10.0. [#1171]
- Release 2.12.0
  * Added ability to display title as a comment in using the info()
    functionality. [#1138]
  * Add ability to set asdf-standard version for schema example
    items. [#1143]
- Add asdf-pr1185+pr1203-fix-jsonschema.patch
  * gh#asdf-format/asdf#1185, gh#asdf-format/asdf#1203
- Add asdf-pr1214-inst
mbakke pushed a commit to guix-mirror/guix that referenced this pull request Nov 21, 2022
* gnu/packages/astronomy.scm (python-gwcs): Fix build and update to 0.18.2.
[build-system]: Use pyproject-build-system.
[arguments]: Remove redundant.
[native-inputs]: Use python-jsonschema-next over python-jsonschema to fix
tests failing to run due to python-asdf issue with low version of jsonschema,
see asdf-format/asdf#1203.

Signed-off-by: Christopher Baines <[email protected]>
@braingram braingram deleted the feature/jsonschema_4.10 branch January 20, 2023 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants