-
-
Notifications
You must be signed in to change notification settings - Fork 584
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
WIP: Support namedtuple validation #365
Conversation
@@ -6,3 +6,10 @@ _static | |||
_templates | |||
|
|||
TODO | |||
|
|||
_trial_temp/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is all stuff created when running tox
locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason these aren't here already is because generally I'd recommend these go in your global git ignore, since they're things that should be ignored from all repos. E.g., mine is: https://github.com/Julian/dotfiles/blob/master/.config/git/ignore
But tbh I'm fine to merge it anyhow since I have had to explain ^ quite a few times so clearly at least some people are tripped up by it.
@@ -135,7 +135,15 @@ def validate(self, *args, **kwargs): | |||
raise error | |||
|
|||
def is_type(self, instance, type): | |||
if type not in self._types: | |||
if type == u"object": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should check for object
in default_types
first, and fall back to this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I'd rather widen the interface entirely here, rather than add complexity to the implementation.
In draft 6 for example, for some reason we decided to make 1.0
be an integer now, so doing isinstance(float)
is no longer a reasonable implementation to check types.
That use case, combined with yours, makes me think that what we'll need to do now is to actually move away from the idea that types are a mapping between names and classes to check via isinstance, and that we'll have to just have e.g. Validator(is_type=SomeInjectableFunction)
where the function will be passed 2 arguments, the same 2 as this is_type method. In your case for namedtuple, you'd inject in a function that first checks if it has your _asdict
attribute and then calls up to the default implementation.
Does that sound reasonable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I'll see what I can do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure you're still interested in this, but if you are, @bsmithers is also poking about in this area in a branch of his. I think he's made some progress on the track above -- if you're interested might want to sync up with him.
Gonna close this PR in the hope that we'll make enough progress there to resolve #364 but yeah this was super appreciated so definitely wouldn't mind some more hands if you are still pitching in.
817b724 add perl implementation and test suite to the user list ca14e01 Merge branch 'pull/382' 3dabf55 move non-format tests out of the format directory, while keeping all ECMA regex tests together 4121aa5 move format tests to their own directory 4bae8aa Add more idn-hostname tests to draft-2019-09 and draft-07 6d91158 [325] Add some more hostname tests e593057 Merge pull request #389 from ssilverman/readme-drafts fb3766d README: Improve issue/PR links 79bef22 README: Update language around drafts ade47e4 README: Add Snow to the list of supporting Java validators fc0c14e README: Update simple JSON example 1167669 README: Update structure, consistency, spacing, and wrapping 9514122 Merge pull request #388 from json-schema-org/ether/maxProperties=0 7646490 test that maxProperties = 0 means the object is empty c3f4319 Merge pull request #384 from ChALkeR/chalker/unique 7766f48 Improve uniqueItems validation 7555d41 Add unnormalized $id tests 11f70eb [300] Add tests for valid use of empty fragments in "$id" b106ff0 [299] Add tests for invalid use of fragments in "$id" 4a74d45 Fix "tilde" spelling 3eca41b Merge pull request #379 from json-schema-org/ether/remove-wrapped-refs d61bae8 remove wrapped refs 536ec07 [359] Add unevaluatedProperties/unevaluatedItems cousin tests ac63eb7 Small README update that introduces the concept of directories 697944e Merge pull request #374 from karenetheridge/ether/allOf-anyOf-oneOf 33f8549 test all the *Of keywords together 44b99ed Merge pull request #373 from karenetheridge/ether/items-and-contains 4a2b52f some tests of items + contains 7f00cc8 add test that was present for other drafts but not 2019-09 a3f9e2e Merge pull request #371 from karenetheridge/ether/if-then-else-boolean aeeaecc some tests with if/then/else and boolean schemas b8a083c Merge pull request #372 from nomnoms12/unique-false-and-zero 85728f1 Add tests for uniqueness [1] and [true] fd01a60 Add tests for uniqueness [1] and [true] 0a8823c Merge pull request #370 from karenetheridge/ether/nul-char fa6f4dd add tests for the NUL character in strings 8bf2f7f Merge pull request #369 from ssilverman/data-desc 2ba7a76 Add data description 4f66078 Merge pull request #367 from karenetheridge/ether/additionalItems 283da7c some more tests for additionalItems 7ba95f3 add tests for keywords maxContains, minContains 2f2e7cf Merge pull request #365 from karenetheridge/ether/move-ecma-regex 8388f27 move ECMA regex tests under format/ git-subtree-dir: json git-subtree-split: 817b724b7a64d7c18a8232aa32b5f1cc1d6dd153
See #364
I'm not sure if this is enough because, though the tests pass, I'm not sure they are written correctly.