-
Notifications
You must be signed in to change notification settings - Fork 73
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
Prepare for 0.11.0. #86
Conversation
This adds support for 2.13.0-M1. It also uses Play-Json 2.6.0 for 2.12+, which finally gets all our support projects using all the "stable" cross versions of Scala we support. Finally, it bumps the benchmarks to 2.12. Thanks to Guillaume Massé for PR #73 which includes some of these changes.
This PR supersedes #73. |
It would be nice to add some methods in Jawn that will go from |
This is a really big commit. It does a bunch of things: - add Util.parseLong and Util.parseLongUnsafe - add jawn.Slice, a CharSequence implementation - add jawn.CharSequenceParser, another parser - add benchmarks for Util.parseLong, etc. - add tests and properties for new code - update facades where appropriate After this commit, users should be in a position to take advantage of using CharSequences instead of Strings where possible, and also to take advantage of faster parsing to Long values.
The +lines changed are a bit worrying, but on the other hand they are overwhelmingly tests and benchmarks, which seem 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.
Looks good to me in general. It feels maybe a little strange to introduce this Util
into jawn-parser, since it's not used anywhere there, and since it seems like it's likely to have a fairly narrow audience—i.e. people for whom .toString.toLong
is too slow but who don't need more sophisticated parsing to handle integral JSON numbers outside the range of Long
.
* wrapping a very large string, garbage collection on the underlying | ||
* string will not occur until all slices are freed. | ||
*/ | ||
final class Slice private[jawn] (s: String, start: Int, limit: Int) extends CharSequence { |
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 know it's "inappropriate to use arbitrary CharSequence
instances as elements in a set or as keys in a map" but do you think this should have a reasonable equals
/ hashCode
anyway?
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.
Also I'm not sure it's necessary but I'm now paranoid about Serializable
, and I could imagine that if circe ended up using these in its JSON string representation we'd want them to be Serializable
.
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.
Yeah, I think this all makes sense.
For equality, I will probably use x.toString == y.toString
, which seems like a reasonable compromise (since I don't want to expose internal structure).
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.
Right, that seems like a reasonable equals
to me.
I think I am going to take @travisbrown's suggestion and move |
This removes Slice and the parseLong* methods from jawn-parser, adds them to jawn-util, adds some better tests and documentation, and adds a dependency on jawn-util to jawn-ast and to jawn-json4s.
@travisbrown See what you think about the changes I just pushed. I'm not going to get this released until Sunday at the earliest, but apart from README updates I feel pretty good about where things are in this branch. |
@non Looks good to me! I think to fix the build you just need to add |
Just released 0.11.0. Should be available from Maven Central in a few hours. |
@non Looking in Maven Central it doesn't look like any 2.13 release was made: http://repo1.maven.org/maven2/org/spire-math/ Am I missing something? |
This adds support for 2.13.0-M1. It also uses Play-Json 2.6.0 for 2.12+,
which finally gets all our support projects using all the "stable"
cross versions of Scala we support.
Finally, it bumps the benchmarks to 2.12.
Thanks to Guillaume Massé for PR #73 which includes some of these changes.