Skip to content

Add extra_info on ValidationError for required missing property #856

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

Closed
wants to merge 6 commits into from

Conversation

TsankoBg
Copy link

This PR adds an extra_info attribute on ValidationError, which is used to indicate the missing property in errors from the required validator.

This should resolve issue #119, and can be used for adding details to other validators as well, in the future.

@codecov
Copy link

codecov bot commented Oct 12, 2021

Codecov Report

Merging #856 (6474476) into main (2cf3dc2) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #856      +/-   ##
==========================================
+ Coverage   98.29%   98.31%   +0.01%     
==========================================
  Files          19       19              
  Lines        3111     3144      +33     
  Branches      377      379       +2     
==========================================
+ Hits         3058     3091      +33     
  Misses         41       41              
  Partials       12       12              
Impacted Files Coverage Δ
jsonschema/_utils.py 98.22% <100.00%> (+0.04%) ⬆️
jsonschema/_validators.py 98.45% <100.00%> (+<0.01%) ⬆️
jsonschema/exceptions.py 97.91% <100.00%> (+0.01%) ⬆️
jsonschema/tests/test_validators.py 98.84% <100.00%> (+0.03%) ⬆️

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 2cf3dc2...6474476. Read the comment docs.

@Julian
Copy link
Member

Julian commented Oct 12, 2021

Thanks! I'll leave detailed notes in a bit, but really happy to see this getting addressed. Two immediate things:

@TsankoBg
Copy link
Author

  • Can you have a look at each of those and make sure they're covered as well?

Yes, I will check them out.

@TsankoBg
Copy link
Author

I looked only into: required, additionalProperties, propertyNames, dependentRequired.

Or would you like all named properties to have the extra info attribute? My initial idea was to add it only to the ones in which the path is empty and the property name cannot be retrieved directly.

@Julian
Copy link
Member

Julian commented Oct 14, 2021

No I just meant those indeed! That sounds great, will have a look.

@Julian
Copy link
Member

Julian commented Sep 9, 2022

@TsankoBg with sincere apologies (since you yourself did everything right here), some other things have recently convinced me to bite the bullet on finally doing #119 (comment) (i.e. doing this via separating out ValidationError). I have some code locally which does so, but needs some tidying. I'm going to close this as yeah I think that's a better direction long term, but I sincerely appreciate sending the PR for what is the longest-running issue here :(

@Julian Julian closed this Sep 9, 2022
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.

2 participants