Validationerror context #86

gazpachoking · 2013-03-29T05:42:07Z

Moving this to a pull for easier tracking of changes. Adds more attributes to ValidationError as discussed in #79

…idationErrors

Add first unit test for new error properties

Refactor ValidationError creation and attribute setting

gazpachoking · 2013-03-29T23:33:32Z

I ended up refactoring a few more things, let me know if you are in to it, I took out most of the arguments from ValidationError.__init__, as they should be set by iter_errors, not the validator functions themselves. Draft 3 requirements is the only exception, (that validator also sucks because it's the only one where path points to a non existing place in the instance.) I also changed the ValidationError -> SchemaError conversion method so as to grab all attributes, not sure if it's too hacky, maybe it should just be changed to manually copy all of the attributes.

gazpachoking · 2013-03-30T03:13:05Z

jsonschema.py

-                error.message, validator=error.validator, path=error.path,
-            )
+            schema_error = SchemaError(error.message)
+            schema_error.__dict__.update(error.__dict__)


Not sure if all the properties should be manually transferred instead...

I'd prefer that, yeah.

Mostly the reason __init__ took parameters to begin with was to allow constructing SchemaErrors from ValidationErrors and passing them the existing parameters..

gazpachoking · 2013-04-01T18:38:46Z

Okay, I updated the docs too, the wording may need tweaked on a couple of these though.

Julian · 2013-04-03T04:25:22Z

jsonschema.py

-                    if error.validator is None:
-                        error.validator = k
+                    # set details if they weren't already set by the called fn
+                    error.set_details(k, v, instance, _schema)


Nitpick: these names are kind of uninformative, so let's use keyword args to pass these here.

gazpachoking · 2013-04-03T04:46:20Z

Yeah, I had considered that too, I think it does sound a bit better.

Julian · 2013-04-03T05:22:14Z

docs/errors.rst

+        from the subschemas will be available on this property. The
+        ``schema_path`` and ``path`` of these errors will be relative to the
+        parent error.
+


I think it'd be helpful if we also added one more (very short) prose section to this file showing an example using all these new attributes. Say something that used anyOf or one of those validators and pulled off this information from it.

Okay, I added a bit on this, not sure if it's what you had in mind, documentation is not my strong suit. :P

We don't actually need an attribute for the instance do we? The caller knows what they passed in, no?

I think we should keep it, the instance you pass in may be a deeply nested structure, the instance on the error is the one that was passed into the validator that failed. You could trace it out using the path within the instance you passed in, I just put it there because it seems much more convenient to see what actual failed the specific validator without having to find it in the original instance.

Ah OK we should make that clear.
On Apr 9, 2013 11:22 PM, "Chase Sterling" [email protected] wrote:

In docs/errors.rst:

.. attribute:: path A deque containing the path to the offending element (or an empty deque if the error happened globally).

.. attribute:: instance

The instance that was being validated.

.. attribute:: context

If the error was caused by errors in subschemas, the list of errors

from the subschemas will be available on this property. The

`schema_path` and `path` of these errors will be relative to the

parent error.

I think we should keep it, the instance you pass in may be a deeply nested
structure, the instance on the error is the one that was passed into the
validator that failed. You could trace it out using the path within the
instance you passed in, I just put it there because it seems much more
convenient to see what actual failed the specific validator without having
to find it in the original instance.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/86/files#r3727202
.

gazpachoking · 2013-04-06T00:27:29Z

I believe that should sort out the hash ordering issues.

Julian · 2013-04-07T00:51:29Z

I actually only meant in the documentation that's being added, but if the tests relied on it that definitely is worth changing too.

Anyways, I'll take another pass over this to double check but think we're almost done here. Thanks :D.

Julian · 2013-04-07T03:48:46Z

jsonschema.py

@@ -56,12 +57,34 @@


 class _Error(Exception):
-    def __init__(self, message, validator=None, path=(), cause=None):


What was the reason why we're not having all the new stuff be arguments to __init__ again?

Also, I think we should make Validator.to_schema_error instead be _Error.create_from and then do SchemaError.create_from(validation_error).

Oh and, the call to super (which for Exception.__init__ just puts all the stuff you give it in .args), all the args to __init__ (whatever those may be) should be there.

The stuff I left in __init__ should be set on creation time, the paths are modified by iter_errors, or descend, and the rest should be set by set_details. The only real exception to this is the required keyword from draft 3, and that's because it's being validated from outside validate_required. Those were my thoughts at least, I'm fine with switching it though.

OK, no that sounds fine I think. As long as we have create_from / to_validation_error it doesn't really matter much.

I think for backwards compatibility reasons we'll need to leave at least the arguments that were there originally, despite the fact that I agree with your division. And while we're doing that we may as well just have all of them there. It looks like it'd clean up tests a tiny bit too, where we construct fully ready errors.

I started merging, so I can do it.

That sounds fine, we need to be a bit careful though, as the properties set by set_details should only be set once, and at the same time, (so that when the error gets passed down the chain they aren't overwritten) so if somebody put some of the details in the constructor, then tried to call set_details...

In fact, I think users shouldn't really be calling set_details, or setting the properties it sets, as they are supposed to be the exact properties that were passed into the validation func. The exception is draft 3 required keyword, because it's validating a keyword on a subschema that is never being validated itself (I think that's sortof a design mistake with draft 3, and one of the reasons it was changed for draft 4.) I am unsure whether users may want to write other validator funcs that work similarly though...

I agree. We can document that. Probably also can make this private - even if they do want to change the behavior they can just set the attributes.

Julian · 2013-04-10T04:15:16Z

OK, merged, and did some tidying.

Check it out, if it looks OK we can do a release tomorrow 🎆.

gazpachoking · 2013-04-10T04:52:34Z

Sweet, everything looked good to me at first glance. I'll try to take a closer peek tomorrow, but I didn't see anything that would hold up a release. I'm a little hesitant about the _set stuff, but it shouldn't be needed by users, and your solution seems pretty good to keep backwards compatibility, and I don't have any better ideas at the moment.

gazpachoking · 2013-04-10T13:04:47Z

I changed my mind about the _set stuff after sleeping on it. I like that solution quite a bit now. 👍

Julian · 2013-04-10T13:08:08Z

:) awesome.

Julian · 2013-04-11T00:39:21Z

docs/errors.rst

@@ -15,15 +15,102 @@ raised or returned, depending on which method or function is used.

        A human readable message explaining the error.

-    .. attribute:: validator
+    .. attribute:: validator_keyword


Despite the fact that I also like validator_keyword better, I think to avoid the API churn we should leave it as is. It's going to be a pain -- we couldn't remove validator until at least v2.0 and it's just going to annoy for a tiny purity benefit.

You da boss. 😄 That's fine by me.

coveralls · 2014-06-11T14:03:23Z

Changes Unknown when pulling 16ec13c on gazpachoking:validationerror_context into * on Julian:master*.

coveralls · 2014-06-12T16:59:43Z

Changes Unknown when pulling 16ec13c on gazpachoking:validationerror_context into * on Julian:master*.

0b657e8 Merge pull request #86 from kylef/patch-1 1bd4151 [README] JSONSchema.swift uses these tests too 8f86716 Revert "Add jon, JSON parser for the fishshell." db9c629 Merge pull request #82 from bucaran/patch-1 875fa42 Add jon, JSON parser for the fishshell. 64c556c Merge pull request #81 from s-panferov/patch-1 43105d4 Add new Rust library to the list aa4d927 Merge pull request #80 from seagreen/implementations 20ef067 Add new haskell implementation. 1274070 Merge pull request #79 from Muscula/json-schema-benchmark 6d8cf45 Merge pull request #78 from JamesNK/patch-1 55c4992 Add json-schema-benchmark to list of users of this test suite 645623d Added Newtonsoft.Json.Schema implementation a7944d1 Merge pull request #76 from Prestaul/patch-1 5729cdf Added skeemas to list of suite users 4600fe3 Make the implementation list a bit less unwieldy now that it's so long (hooray!) 11d6905 Merge remote-tracking branch 'mafintosh/patch-1' into develop 689b80f Merge pull request #74 from bugventure/develop c36f888 Add request-validator as a user of the test suite 41876b1 Update README.md aabcb34 Merge pull request #71 from seagreen/additionalproperties b3d160b Add tests for additionalProperties by itself. git-subtree-dir: json git-subtree-split: 0b657e8b0d21a4099d3ce378ba7208073a9295e6

gazpachoking added 4 commits March 17, 2013 23:42

Add schema_path attribute to ValidationErrors

54f16a7

Add validator_arg, instance, subschema, and context attributes to Val…

741c275

…idationErrors

Fix issue with new error properties on nested errors

cf20b09

Add first unit test for new error properties

Rename some ValidationError attributes

77ffe31

Refactor ValidationError creation and attribute setting

gazpachoking reviewed Mar 30, 2013
View reviewed changes

gazpachoking added 2 commits April 1, 2013 14:09

Add anyOf test for the new ValidationError properties

300f389

Update the docs for ValidationError with the new attributes

7234ec3

Julian reviewed Apr 3, 2013
View reviewed changes

gazpachoking added 2 commits April 3, 2013 00:36

Manually transfer ValidationError attributes to SchemaError

951ee65

Use keyword args to set ValidationError details for clarity

d493bdb

Make a method to create SchemaErrors from ValidationErrors

ff74103

Julian reviewed Apr 3, 2013
View reviewed changes

gazpachoking added 5 commits April 4, 2013 00:10

Implement IValidator.extends for subschema validation within validators

45e20de

Update docs for descend method

af86042

Add example of using the ValidationError attributes

1a74234

Don't rely on hash sorting for docs examples

832f51c

Make sure sorted_errors has unambiguous order for all error lists

d3c6f27

Make sure unlike types can't get compared in sorted_errors again

ceede42

Julian reviewed Apr 7, 2013
View reviewed changes

gazpachoking added 3 commits April 7, 2013 00:05

Add missing paren in docs

0e49379

Refactor ValidationError.to_schema_error to _Error.create_from

4fe2a8b

Pass all arguments to super in _Error

cf7056d

Use python 3 style prints in docs

16ec13c

gazpachoking mentioned this pull request Apr 8, 2013

Report multiple 'leaf' errors rather than one schema error #51

Closed

Julian merged commit 16ec13c into python-jsonschema:master Apr 10, 2013

Julian reviewed Apr 11, 2013
View reviewed changes

gazpachoking deleted the validationerror_context branch April 11, 2013 01:32

		@@ -56,12 +57,34 @@


		class _Error(Exception):
		def __init__(self, message, validator=None, path=(), cause=None):

Uh oh!

Validationerror context #86

Validationerror context #86

Uh oh!

Conversation

gazpachoking commented Mar 29, 2013

Uh oh!

gazpachoking commented Mar 29, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gazpachoking commented Apr 1, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gazpachoking commented Apr 3, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gazpachoking commented Apr 6, 2013

Uh oh!

Julian commented Apr 7, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Julian commented Apr 10, 2013

Uh oh!

gazpachoking commented Apr 10, 2013

Uh oh!

gazpachoking commented Apr 10, 2013

Uh oh!

Julian commented Apr 10, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jun 11, 2014

Uh oh!

coveralls commented Jun 12, 2014

Uh oh!

Uh oh!