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

Move warnings to user context #899

Merged
merged 1 commit into from Jan 1, 2022
Merged

Move warnings to user context #899

merged 1 commit into from Jan 1, 2022

Conversation

Kojoley
Copy link
Contributor

@Kojoley Kojoley commented Jan 1, 2022

Currently the warning location points inside the jsonschema code and it takes an additional effort to find where the actual code that triggers it is located.

@Julian
Copy link
Member

Julian commented Jan 1, 2022

Thanks, this is definitely reasonable -- can you update the tests for these deprecations?

Appreciated.

@codecov
Copy link

codecov bot commented Jan 1, 2022

Codecov Report

Merging #899 (15d7cb9) into main (405f735) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #899   +/-   ##
=======================================
  Coverage   98.27%   98.27%           
=======================================
  Files          20       20           
  Lines        3181     3188    +7     
  Branches      430      430           
=======================================
+ Hits         3126     3133    +7     
  Misses         44       44           
  Partials       11       11           
Impacted Files Coverage Δ
jsonschema/__init__.py 86.66% <ø> (ø)
jsonschema/validators.py 97.52% <ø> (ø)
jsonschema/tests/test_deprecations.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 405f735...15d7cb9. Read the comment docs.

@Kojoley
Copy link
Contributor Author

Kojoley commented Jan 1, 2022

Thanks, this is definitely reasonable -- can you update the tests for these deprecations?

Appreciated.

I could not figure out what should I update, the test suit seems to be fine as-is, may you please explain what do you mean?

@Julian
Copy link
Member

Julian commented Jan 1, 2022

Each deprecation here has a test asserting that some code triggers the deprecation warning.
That test should also be modified so not only does it assert the deprecation was raised, but also that the stacklevel was correct (which it wasn't before this change).
I'm not at a computer but I can give an example later if needed, just let me know if the above is unclear. (But essentially every change here should have a test ensuring the change was broken before and fixed now)

Currently the warning location points inside the jsonschema code and it takes an additional effort to find where the actual code that triggers it is located.
@Kojoley
Copy link
Contributor Author

Kojoley commented Jan 1, 2022

Should be done now.

@Julian
Copy link
Member

Julian commented Jan 1, 2022

Wonderful, thanks!

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.

None yet

2 participants