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

Make sure empty fragment and no fragment are the same in $schema #77

Merged

Conversation

gazpachoking
Copy link
Contributor

My idea for the issue in #73


"""

if uri:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this? Is this to handle a uri of ""? If so then it then turns it into None which is almost certainly not what we want.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this was to counter a bug? in urlparse.defrag, when an empty uri was passed in it returned a tuple of empty byte strings rather than unicode strings, which could not be concatenated with the unicode '#'. If it returns '' when the uri given was empty is that better?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like it returns whatever type you pass in, no? I get back unicode for unicode uris and bytes for bytes uris on both Py2 and Py3?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to double check. I just know it was crashing on the tests without that conditional.

@Julian
Copy link
Member

Julian commented Mar 3, 2013

Other than my comment this is fine as is but maybe we should look into a Reference class if that would give us any benefit. E.g. as-is you can't look up a meta schema URI in the meta-schemas dict without knowing what our canonical way of storing it is.

@gazpachoking
Copy link
Contributor Author

Maybe we should use urlparse.urlsplit(uri).geturl() to create the canonical form, this also gets us some other normalization, like lower casing the scheme.

@Julian
Copy link
Member

Julian commented Mar 3, 2013

Sounds good. Makes me even more likely to think that we need to wrap all this somehow though in a way that users can actually transparently use.

@gazpachoking
Copy link
Contributor Author

I think I agree, not quite sure how you are thinking to do that though.

@Julian
Copy link
Member

Julian commented Mar 3, 2013

Haven't thought it fully through yet, but probably with a combination of a Reference class that normalizes the uri you give it (and normalizes uris that are strs that you compare to it), and having the meta_schemas dict have References for keys (and we can decide whether it should be smarter than that and actually wrap strs that you check it for with References as well, though for our purposes right now I doubt we even need that).

Open for better ideas though.

@gazpachoking
Copy link
Contributor Author

Hmm, yeah, we should probably use something like this when adding and looking up in the RefResolver store too.

@gazpachoking
Copy link
Contributor Author

What if we made the metaschema dict, and the RefResolver store into something like this:

class URIDict(dict):
    def normalize(self, uri):
        return urlparse.urlsplit(uri).geturl()

    def __getitem__(self, uri):
        return super(URIDict, self).__getitem__(self.normalize(uri))

    def __setitem__(self, uri, value):
        super(URIDict, self).__setitem__(self.normalize(uri), value)

This isn't quite complete, due to not having init, delitem etc. defined, trying to figure if there is an easier way to do all that than just redefine them all.

@Julian
Copy link
Member

Julian commented Mar 3, 2013

Right that was what I meant by

(and we can decide whether it should be smarter than that and actually wrap strs that you check it for with References as well, though for our purposes right now I doubt we even need that)

We'd want to do it using collections.abc.MutableMapping as opposed to subclassing though.

@gazpachoking
Copy link
Contributor Author

Okay, gotcha. Thought you meant something along those lines. Well, I think it's a good plan, sucks something like this has to redefine all the dict methods though, seems like there should be some way to provide a normalization function and get all this functionality.

@gazpachoking
Copy link
Contributor Author

What do we gain by using MutableMapping? Seems similar, except for we'd have to have our own dict store within it.

@Julian
Copy link
Member

Julian commented Mar 3, 2013

Subclassing built in types is unreliable ( for the same reason subclassing
in general is unreliable) because it relies on knowledge about the
implementation of your superclass. E.g. If you implement getitem to
normalize, what happens if you use .get? Or setitem and .update (it
turns out CPython gets that right now in those 2 specific cases).

So MutableMapping makes sure all of that happens correctly as long as you
implement the interface correctly for all the methods it expects.
On Mar 2, 2013 11:14 PM, "Chase Sterling" notifications@github.com wrote:

What do we gain by using MutableMapping? Seems similar, except for we'd
have to have our own dict store within it.


Reply to this email directly or view it on GitHubhttps://github.com//pull/77#issuecomment-14341293
.

@gazpachoking
Copy link
Contributor Author

Okay, MutableMapping gives us those things? I thought we still had to define all of those methods. I'll look into it.

@gazpachoking
Copy link
Contributor Author

Ahh, seems it does. Nice, well in that case, I'm all for it, good call. :)

@gazpachoking
Copy link
Contributor Author

Sucks a little that we need that much code for a simple problem, but is this what you were thinking?

@gazpachoking
Copy link
Contributor Author

Oh, good call. Seems I need to fix something on python 2.7 too.

@Julian
Copy link
Member

Julian commented Mar 3, 2013

Yeah it does a bit. But yeah looks like it. We should probably make it private too (So _URIDict). Otherwise looks good.

@@ -1235,6 +1270,7 @@ def _uniq(container):

def validate(instance, schema, cls=None, *args, **kwargs):
if cls is None:
cls = meta_schemas.get(schema.get("$schema"), Draft4Validator)
cls = meta_schemas.get(schema.get("$schema"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this line is unchanged now.

@gazpachoking
Copy link
Contributor Author

Okay, think I got everything sorted now.

gazpachoking added a commit that referenced this pull request Mar 3, 2013
Make sure empty fragment and no fragment are the same in $schema
@gazpachoking gazpachoking merged commit c46ce21 into python-jsonschema:master Mar 3, 2013
@gazpachoking gazpachoking deleted the canonical_schema_uris branch March 3, 2013 05:28
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