Skip to content

Sort extra properties for additionalProperties errors #774

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

Conversation

robherring
Copy link
Contributor

extras is a set which is unordered. Let's sort it so users get
consistent error messages.

Signed-off-by: Rob Herring [email protected]

extras is a set which is unordered. Let's sort it so users get
consistent error messages.

Signed-off-by: Rob Herring <[email protected]>
@codecov
Copy link

codecov bot commented Jan 26, 2021

Codecov Report

Merging #774 (b823ef5) into main (9814afc) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #774   +/-   ##
=======================================
  Coverage   96.37%   96.37%           
=======================================
  Files          17       17           
  Lines        2703     2703           
  Branches      309      309           
=======================================
  Hits         2605     2605           
  Misses         79       79           
  Partials       19       19           
Impacted Files Coverage Δ
jsonschema/_validators.py 99.54% <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 9814afc...b823ef5. Read the comment docs.

@Julian
Copy link
Member

Julian commented Jan 28, 2021

Hi, can you elaborate on what problems the error messages not being in consistent sort order causes?

@Julian
Copy link
Member

Julian commented Jan 31, 2021

Closing since I'm not sure I follow what the idea here would be, but happy to discuss further.

@Julian Julian closed this Jan 31, 2021
@robherring
Copy link
Contributor Author

Users (of dtschema) have reported to me that the order changes from run to run and diffing error logs shows differences. So it's problematic to have that noise.

Also note that 'extras' is sorted in the case with 'patternProperties' just above, but not here.

@Julian
Copy link
Member

Julian commented Feb 4, 2021

The exact contents of error messages aren't part of jsonschema's public API -- so patternProperties's extras may become unsorted at any point in time if it somehow becomes required to do so.

It sounds like perhaps these users should instead be logging the fields off of ValidationErrors that they want to track in a structured way, rather than relying on the precise error message, which as mentioned isn't guaranteed to look any certain way or another.

(Though #119 may be relevant here)

@robherring
Copy link
Contributor Author

The exact contents of error messages aren't part of jsonschema's public API -- so patternProperties's extras may become unsorted at any point in time if it somehow becomes required to do so.

I'm only asking for it to be stable within a given release, not across releases.

It sounds like perhaps these users should instead be logging the fields off of ValidationErrors that they want to track in a structured way, rather than relying on the precise error message, which as mentioned isn't guaranteed to look any certain way or another.

(Though #119 may be relevant here)

Exactly, the data in question isn't in a structured field to extract it.

@Julian
Copy link
Member

Julian commented Feb 5, 2021

Exactly, the data in question isn't in a structured field to extract it.

Got it -- then I think the solution is to implement #119 -- there's I believe a specced solution there in the comments if you were interested in giving it a shot (or if further discussion is needed obviously happy to do that).

Even within one release, there's yeah just no guarantees provided here.

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