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

Update 3.x to include current master #347

Closed
wants to merge 61 commits into from
Closed

Update 3.x to include current master #347

wants to merge 61 commits into from

Conversation

RST-J
Copy link
Contributor

@RST-J RST-J commented Jul 13, 2016

I merged master into our very stale 3.x branch. There are some spots I'm not sure about, I'll comment them directly (it was only one).
@iainbeeston In general we should come up with a decision about how to go on with v3. What is your current opinion on that?

iainbeeston and others added 30 commits March 6, 2015 13:14
There are a lot of places in the code where we rescue any error, at all.
That's quite dangerous, as it could conceal bugs.

I've changed it so that we always specify which error class we want to
catch. Mostly it's been a minor change, but there are two API changes:

* When it comes to loading data I've had to introduce an explicit list
  of protocols which we can load json over (otherwise it's possible to
  have a uri with a scheme that makes no sense - it'd still be a valid
  uri but loading from it is impossible). I've used the list of
  protocols that addressable recognizes for now.
* No matter what JSON parser you use, we now always raise a
  JSON::Schema::JsonParseError. Without doing this it would be tricky to
  handle parser errors identically, for all parsers (the other option
  would be to catch a long list of potential parse errors, but this
  seems more sensible).
Older versions raised ruby warnings
Explicitly notes :strict overrides any required properties set in schema
…dator

register_format_validator on default_validator
…dator

Test all versions in test_custom_format
Update README.textile to fix schema validation example
Addressible's URI parsing shows up at the top of the profiler for our use case.

This patch adds a cache to JSON::Util::URI.normalized_uri, to trade CPU time for memory.

There are other expensive Addressible methods that should be cached elsewhere.

benchmark: https://github.com/mjc/json-schema-perf

MRI 2.2.2 before: 825.363  (± 6.2%) i/s
MRI 2.2.2 after:  1.436k (± 4.5%) i/s

1.74x faster

JRuby 1.7.20 before: 1.445k (± 2.5%) i/s
JRuby 1.7.20 after: 2.272k (± 6.1%) i/s

1.57x faster
Addressable::URI#fragment= triggers a revalidation of the URL, which is
super slow.  Don't do this when it isn't necessary.
Hopefully this will make it easier to change in future
For the unused variable `validator`
RST-J and others added 23 commits February 12, 2016 18:50
This started as a straight up automated conversion via pandoc:

  pandoc README.textile -o README.md

I then followed up by fixing the images it did not convert properly,
removing extraneous backslashes that it added, and adding finced code
blocks for syntax highlighting. I also replaced <code>...</code>
with backticks and adjusted some whitespace.
Making it a little easier to read the descriptions of the three validate methods.
Reformatted the examples to make them easier to read
Turns out that before:

* if you had a ref that included a colon, the text before the colon was
  treated as the scheme and the text after the colon as a path
* the unescape_uri method was actually only parsing the path of the
  uri (therefore anything before a colon was being stripped when passed
  to unescape_uri)

Fixes voxpupuli#319
…cters-in-refs

Made it possible to have refs that include URI-special characters
Travis doesn't support 2.3 yet
schema = JSON::Schema.new(JSON::Validator.parse(schema), schema_uri, @options[:version])
if @options[:list] && @options[:fragment].nil?
schema = schema.to_array_schema
end
Validator.add_schema(schema)
rescue
rescue JSON::ParserError
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently in master we have JSON::Schema::JsonParseError being rescued here. But if we only support JSON, I think there is no need to wrap parsing errors any more. Did I miss something?

@RST-J
Copy link
Contributor Author

RST-J commented Jul 13, 2016

The 1.9 job failed because it tries to install a too new json gem version. Does someone now how fix that?

@iainbeeston
Copy link
Contributor

I think that the 3.0 branch is defunct - the two things that were in 3.0 that weren't in master were the removal of ruby 1.8 support, and removing support for alternative json back-ends. We've already removed ruby 1.8 support (in #340 ) and I've extracted out @pd 's commit to remove json backends in #339 . I think that once #339 is merged and released, we can close this PR and delete the 3.0 branch.

I think more generally we should continue making refactoring in master in minor releases, and once we're ready to make breaking changes (like removing caching and class variables) we should do that in a new 3.0 release. But we're not ready for that yet and maintaining a 3.0 branch is hard work.

@iainbeeston
Copy link
Contributor

@RST-J I just noticed the ruby 1.9 build issue too - I'll fix that

@RST-J
Copy link
Contributor Author

RST-J commented Jul 20, 2016

I thought, we already had more in 3.0, but under this circumstances I agree with you.
So we need two phases, the first for cleaning up the code while remaining backwards compatible and then at some point we need to introduce all the changes we think are important for 3.0. That actually could help focusing on the respective goal, rather than doing both at the same time or interleaved.
Nevertheless, when we start developing/merging the breaking changes, this should ideally be within a relativly short time frame or we'll run in the same or a similar problem again.

@iainbeeston
Copy link
Contributor

I agree. Can we close this pull request for now?

I should probably delete the 3.0 branch too - until (as you said) we're ready to make a short set of breaking changes for 3.0.

@RST-J RST-J closed this Jul 27, 2016
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.

8 participants