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

Remove references to RefResolver #1215

Merged
merged 18 commits into from
Sep 19, 2023
Merged

Conversation

jsignell
Copy link
Member

@jsignell jsignell commented Sep 8, 2023

Related Issue(s):

Description:

I am working though the recommendations on https://python-jsonschema.readthedocs.io/en/latest/referencing/ these are the steps as I understand them:

  • Replace RefResolver with Registry
  • Create a callable that loads referenced relative schemas
  • Create a callable that loads referenced absolute schemas

PR Checklist:

  • pre-commit hooks pass locally
  • Tests pass (run scripts/test)
  • Documentation has been updated to reflect changes, if applicable
  • This PR maintains or improves overall codebase code coverage.
  • Changes are added to the CHANGELOG. See the docs for information about adding to the changelog.

Sorry, something went wrong.

@codecov
Copy link

codecov bot commented Sep 13, 2023

Codecov Report

Patch coverage is 76.00% of modified lines.

Files Changed Coverage
pystac/validation/local_validator.py 61.53%
pystac/validation/stac_validator.py 91.30%
pystac/validation/__init__.py 100.00%

📢 Thoughts on this report? Let us know!.

…mas instead

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@jsignell jsignell marked this pull request as ready for review September 13, 2023 17:54
@jsignell
Copy link
Member Author

Ok! I now understand jsonschema a lot better than I did. As part of that I realized that the LocalValidator was overly complex. I removed it. Let me know if you think that needs to be deprecated rather than just straight up removed. It was only added in #1165 and is just instantiated within stac_validator. It was never really intended for public consumption.

@jsignell jsignell requested a review from gadomski September 13, 2023 19:50
@gadomski gadomski linked an issue Sep 13, 2023 that may be closed by this pull request
@gadomski
Copy link
Member

It was never really intended for public consumption.

Yeah, in retrospect pystac.validation.local_validator probably should have been pystac.validation._local_validator. Maybe we "privatize" the whole module and drop a deprecation warning on the "public" version? E.g. (crib version):

# in pystac/validation/local_validator.py

warnings.warn("This module is deprecated and will be removed in v2")
from ._local_validator import *
__all__ = ["the", "things", "we", "need", "to", "export"]

Copy link
Member

@gadomski gadomski left a comment

Choose a reason for hiding this comment

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

Makes sense to me ... I think the only outstanding question is #1215 (comment).

@jsignell
Copy link
Member Author

Hmm the functionality in the old vs new are pretty different. Maybe I should just leave the old local_validator.py alone and toss a deprecation warning on it and then create a new _local_validator.py.

@jsignell jsignell requested a review from gadomski September 18, 2023 13:40
@jsignell
Copy link
Member Author

Ok see what you think about this approach to deprecation. Note that it probably won't have quite as good coverage as main because the deprecated class is untested. I can add some new tests if that is too gross.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Copy link
Member

@gadomski gadomski left a comment

Choose a reason for hiding this comment

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

Just one small nit then we should be good, thanks!

pystac/validation/local_validator.py Outdated Show resolved Hide resolved
@jsignell jsignell requested a review from gadomski September 19, 2023 13:41
@gadomski
Copy link
Member

I can add some new tests if that is too gross.

Nah, tests just for coverage sake on deprecated things is a waste of time IMO :-). LGTM.

@gadomski gadomski added this pull request to the merge queue Sep 19, 2023
Merged via the queue into stac-utils:main with commit e4a17c1 Sep 19, 2023
20 of 22 checks passed
@jsignell jsignell deleted the ref-resolver branch September 19, 2023 19:40
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.

Don't use RefResolver
2 participants