Adds JNumber conversions

[eed3si9n]

Ref https://github.com/mdedetrich/scalajson/pull/38

This implementation is loosely based on that of Circe.

JNumber is internally subdivided into package private seven datatypes JStringDecimal, JBigDecimal, JBigInt, JLong, JInt, JDouble, and JFloat corresponding to the input values passed into JNumber.apply(...). This allows performant roundtrip back to the original number types, if needed.

Note that the parser implementer now has an option of constructing JStringDecimal via JNumber(value: String) or doing the number parsing upfront.

[codecov-io]

Codecov Report

Merging #41 into master will decrease coverage by 18.61%. The diff coverage is 23.46%.

@@             Coverage Diff             @@
##           master      #41       +/-   ##
===========================================
- Coverage   44.68%   26.07%   -18.62%     
===========================================
  Files           5        5               
  Lines         687     1097      +410     
  Branches      133      170       +37     
===========================================
- Hits          307      286       -21     
- Misses        380      811      +431

Impacted Files	Coverage Δ
js/src/main/scala/scalajson/ast/JValue.scala	0% <0%> (ø)	:arrow_up:
...s/src/main/scala/scalajson/ast/unsafe/JValue.scala	0% <0%> (ø)	:arrow_up:
...m/src/main/scala/scalajson/ast/unsafe/JValue.scala	47.29% <39.61%> (-12.22%)	:arrow_down:
jvm/src/main/scala/scalajson/ast/JValue.scala	48.86% <40.6%> (-24.53%)	:arrow_down:
shared/src/main/scala/scalajson/ast/package.scala	16.88% <0%> (-40.89%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 99ee084...5f215c9. Read the comment docs.

[sirthias]

IMHO representing JSON numbers via an ADT like this is the way to go. What's you rationale for making the sub types package private instead of opening up the ADT altogether?

[mdedetrich]

@sirthias I suppose if the internals change then you are not breaking it for end users?

[sirthias]

IMHO this whole effort is about not changing anything, ever. We need to get it almost 100% right from the beginning.

This is not a library that receives new features or other upgrades over time. It's supposed to deliver a minimal and consistent JSON ADT, not more and not less.

That's why simplicity and clarity is quite important in my view. Any special construct, hidden fields, non-case-classes or other "tricks" should be extremely well justified.

[mdedetrich]

This is not a library that receives new features or other upgrades over time. It's supposed to deliver a minimal and consistent JSON ADT, not more and not less.

Completely agree, which is why I am being very careful about a release. Will need @eed3si9n , and possibly @travisbrown to clarify why they are private

[eed3si9n]

What's you rationale for making the sub types package private instead of opening up the ADT altogether?

Unlike Json4s's AST where you get either case class JLong(num: Long) or case class JDecimal(num: BigDecimal) representing the actual number value, we have seven that represents how the JNumber was constructed. The reason why it's there is to avoid conversion cost, and it's almost irrelevant to the consumer of the AST.

For example, suppose I am getting "Twitter fave count", and I assume the number can be represented as Long, depending on the parser the number might be stored as JStringDecimal, JBigDecimal, JLong or JInt etc. The only thing I need to be concerned is can I safely convert it to Long.

[eed3si9n]

IMHO this whole effort is about not changing anything, ever. We need to get it almost 100% right from the beginning.

I agree with the general idea, but we should relax this to keeping strict binary compatibility of the exposed classes.

Other goals are correctness, user-friendliness, and performance. Hiding often allows us to do performance tricks without compromising on the usability of the surface API.

[mdedetrich]

@eed3si9n Thanks, I will have a look at this today/tomorrow

[sirthias]

Other goals are correctness, user-friendliness, and performance. Hiding often allows us to do performance tricks without compromising on the usability of the surface API.

Yes, I fully agree. However, in my view this project is a bit different from ordinary libraries as it provides almost only structure and very little logic (at least I would consider this a goal). If there's no logic we don't really need to hide anything. Also, if the structure is right there is no need for "performance tricks".

My concern with hiding the internal ADT structure is that there will definitely be people that will want to access the underlying inner structure, if only to perf-optimize certain cases on their layer.

If we come up with right structure anyone who doesn't care about details can simply rely on the JNumber interface. But if I really wanted to I could drop down to the fully specialized JNumber ADT and apply dedicated handling to each case. (One example that come to mind would be an optics library that provides AST transformation/manipulation facilities from the outside. Not being able to see the real underlying structure could be a disadvantage.)

[eed3si9n]

@sirthias After talking with @Ichoran on Gitter, I am coming to the idea that both #38 and this PR have feature creeped.

I think you're right that we should try to keep the AST open, and in our case plain String (with regex check).

[mdedetrich]

Regarding #38 , this was an attempt to provide performant JNumber conversions without altering the current API design too much, hence why the JNumber constructor was largely unchanged and we are using a bitflag

[eed3si9n]

Closing this in favor of #42