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

Any ref issue #200

Merged
merged 4 commits into from
Dec 3, 2014
Merged

Any ref issue #200

merged 4 commits into from
Dec 3, 2014

Conversation

RST-J
Copy link
Contributor

@RST-J RST-J commented Nov 30, 2014

This fixes the issues mentioned in #190.
What does it do:

  • Change the tests so that the issue gets triggered at all (was not without required)
  • Move normalized_uri to be a util function that can be used anywhere (actually this would not be necessary anymore - should I revert this?)
  • Unify the structure of keys used for schemas to be absolute URIs without a fragment hash at the end

A consequence of the changes is that the Fake UUIDs also get converted to paths but I think that is Ok. If we ever attempt to load such a fake UUID file URI this has to be a bug I assume. And, I think it is better if we have a uniform way to create and lookup keys. Technically http://foo.bar/some_schema# and http://foo.bar/some_schema have to be considered equal, but schema authors can use one or the other.
As a developer I prefer to be clear about how the keys look like and do not want to reason about whether there is a # or not or whether there should be one or not.

@iainbeeston
Copy link
Contributor

Rather than expose a hash of schemas to the world (which you have to use a special function to get the key for), would the api be easier to use if there was a "get_schema" method (or similar), that expected a uri? Internally we could still use a hash and map the uris. This way the implementation details (ie the hash and the format used for it's keys) could be hidden from the rest of the codebase (and future changes would only appear in one place)

@iainbeeston
Copy link
Contributor

Ok ignore that - that function already exists (schema_for). It's a shame we're passing normalised URIs around so much internally

@iainbeeston
Copy link
Contributor

@RST-J can you point out where in the code we're using normalised URIs instead of uuids now? (I can't see it)

@RST-J
Copy link
Contributor Author

RST-J commented Dec 1, 2014

@iainbeeston What I wanted to say is, that for schemas without ID which get a fake UUID, the key changes from 550e8400-e29b-11d4-a716-446655440000 to "file://#{Dir.pwd}/550e8400-e29b-11d4-a716-446655440000". This works automagically, because the key is derived on registration and lookup.

@RST-J
Copy link
Contributor Author

RST-J commented Dec 1, 2014

For the issue of throwing around normalized URIs, I guess there is some potential to prevent normalizations if we figure out the spots where they are not really necessary and we just do them for needless precaution. I'll have another look at this in the evening (9 am around here ;) ).

@RST-J
Copy link
Contributor Author

RST-J commented Dec 1, 2014

Actually it would be pretty useful to have some performance benchmarks (maybe as an own suite?) to figure out which way to do things is the fastest. Didn't @pd mention this recently?

@iainbeeston
Copy link
Contributor

👍 btw

@RST-J RST-J mentioned this pull request Dec 1, 2014
4 tasks
@RST-J RST-J added this to the v2.5.0 milestone Dec 2, 2014
@RST-J
Copy link
Contributor Author

RST-J commented Dec 3, 2014

Any more opinions? When this was merged we could release the next version :)

@hoxworth
Copy link
Contributor

hoxworth commented Dec 3, 2014

👍, release away!

@RST-J
Copy link
Contributor Author

RST-J commented Dec 3, 2014

Is the gem pushed to rubygems automatically when we release a version here on github or what are the steps to be taken?

@iainbeeston
Copy link
Contributor

You need to do it locally. Check out master, make damn sure the tests pass(!!!). Then normally I'd run rake release (not sure if we have the bundler rake tasks set up on json-schema?).

@RST-J have you been added to the gem owners on rubygems.org? (I think @hoxworth had trouble adding you)

@iainbeeston
Copy link
Contributor

Or let me know and I'll gladly do it

@iainbeeston
Copy link
Contributor

Oh, and we also need to bump the version.

This discussion should probably be on #202

iainbeeston added a commit that referenced this pull request Dec 3, 2014
@iainbeeston iainbeeston merged commit 86412e0 into voxpupuli:master Dec 3, 2014
@RST-J RST-J deleted the any_ref_issue branch December 3, 2014 13:35
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.

3 participants