mdedetrich commented 7 years ago

@sirthias @Ichoran @ktoso @fommil This PR disallows JNumber to be called directly via the constructor as a String (there is a fromString method which returns an Option[JNumber] with the regex check).

The regex check has also been removed from all constructors which are guaranteed to be a number, and since JNumber is a final class, all of the boilerplate methods to make it treated as a case class has been defined (unapply/canEqual/productArity/productElement).

Note that this was only done to the standard AST and not the unsafe one (not sure if it should also be done for unsafe). Note that when combined with https://github.com/mdedetrich/scalajson/issues/17#issuecomment-312140277 its possible to do some include a bit flag field within JNumber which can track if it gets created as a long/int/double/float so we can make efficient .to converters for different number types.

Let me know if this is looking good, and I can merge/release to M4. I think the only contentious point would be whether these changes make sense in the unsafe AST

codecov-io commented 7 years ago

Codecov Report

Merging #30 into master will increase coverage by 0.12%. The diff coverage is 43.9%.

@@            Coverage Diff             @@
##           master      #30      +/-   ##
==========================================
+ Coverage   44.56%   44.68%   +0.12%     
==========================================
  Files           5        5              
  Lines         662      687      +25     
  Branches      128      133       +5     
==========================================
+ Hits          295      307      +12     
- Misses        367      380      +13

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:
shared/src/main/scala/scalajson/ast/package.scala	`57.77% <100%> (ø)`	:arrow_up:
...m/src/main/scala/scalajson/ast/unsafe/JValue.scala	`59.5% <75%> (-0.24%)`	:arrow_down:
jvm/src/main/scala/scalajson/ast/JValue.scala	`73.39% <77.77%> (+0.39%)`	:arrow_up:

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 0c444bf...d75e458. Read the comment docs.

sirthias commented 7 years ago

Why do you prefer the "manual" case class over the actual case class, which is now possible (since 2.11.11 and 2.12.2)?

mdedetrich commented 7 years ago

@sirthias The reason why I used a manual class is because of Scale 2.10.x support. Also there isn't any real difference for the user and we don't have to rely on compiler flags (safer route)

sirthias commented 7 years ago

@sirthias The reason why I used a manual class is because of Scale 2.10.x support. Also there isn't any real difference for the user and we don't have to rely on compiler flags (safer route)

Ok, so it comes down to either supporting Scala 2.10 or going for the standard case class. Let's understand a bit the pros/cons of either choice:

Dropping Scala 2.10 support would be bad because all projects that are still on Scala 2.10 could not depend on ScalaJSON. In order to understand the impact of this it'd be important to understand which projects this are. Do you know of any?
Going for the "manual" case class has a number of (IMHO) important drawbacks:

a) Asymmetry with all the other model classes. It seems weird that everything is a case class except for JNumber. b) Forgoing internal optimizations. AFAIK the compiler treats (final) case classes specially in certain cases (e.g. in pattern matches) and applies a number of internal optimizations, e.g. by removing superfluous allocations (see also the discussion around unapply). The "manual" case class will not be able to benefit from these. c) Risks with regard to meta support. Certain tools like shapeless generics or other macros might require an ADT to be modelled from actual cases classes to work as expected. I am quite sure that going down the manual route will hurt us here in some way or another at some point in the future! d) Less future proof. Coming developments (e.g. around TASTY) might add even more significance to case classes in unforeseen ways. The asymmetry in the ScalaJSON ADT is not a good thing here either. e) We have quite a few more lines of code in ScalaJSON, which makes the code harder to read and maintain. For example, we have the risk that the compiler changes the default desugaring of case classes in subtle ways (between versions) that we won't (immediately) see and follow. f) People reading the code will always and automatically look at the code and try to spot the difference to what they think the default desugaring of case classes is, which is hard, because many people will not know the details to well. Overall it's much more confusing that a standard case class.

So, overall I think we need to have very good reasons to not go for the standard case class. In my eyes, it comes down to fully understanding how important Scala 2.10 support really is. My guess is that it is of no significance whatsoever, but I might be wrong.

mdedetrich commented 7 years ago

@sirthias

Dropping Scala 2.10 support would be bad because all projects that are still on Scala 2.10 could not depend on ScalaJSON. In order to understand the impact of this it'd be important to understand which projects this are. Do you know of any?

http4s, apparently they are going to be stuck on 2.10 for a while. Also circe (due to http4s and cats)

Asymmetry with all the other model classes. It seems weird that everything is a case class except for JNumber.

Afaik, the asymmetry is only in the source and not how end users end up using the JNumber class (the only difference at this point is the new keyword, which end-users don't even use because they can't construct the JNumber directly with a string anyways)

Forgoing internal optimizations. AFAIK the compiler treats (final) case classes specially in certain cases (e.g. in pattern matches) and applies a number of internal optimizations, e.g. by removing superfluous allocations (see also the discussion around unapply). The "manual" case class will not be able to benefit from these.

This is probably the most convincing argument, but I am not completely aware of these optimizations. Also I think you can simulate the changes for unapply in a manual class

Risks with regard to meta support. Certain tools like shapeless generics or other macros might require an ADT to be modelled from actual cases classes to work as expected. I am quite sure that going down the manual route will hurt us here in some way or another at some point in the future!

Less future proof. Coming developments (e.g. around TASTY) might add even more significance to case classes in unforeseen ways. The asymmetry in the ScalaJSON ADT is not a good thing here either.

This is true

People reading the code will always and automatically look at the code and try to spot the difference to what they think the default desugaring of case classes is, which is hard, because many people will not know the details to well. Overall it's much more confusing that a standard case class.

This is a documentation issue

So, overall I think we need to have very good reasons to not go for the standard case class. In my eyes, it comes down to fully understanding how important Scala 2.10 support really is. My guess is that it is of no significance whatsoever, but I might be wrong.

Unfortunately its very significant (http4s/circe), but there are alternatives

Provide the current implementation just for scala 2.10.x. Means more code to maintain (although not a real issue with a library that is meant to rarely change) and use the case class for 2.11.x/2.12.x
Just use a case class for the entire codebase and present the Scala 2.10 users with a big warning label that they can access the constructor themselves

Also with these compiler flags that are needed for Scala 2.11.x, I assume its ScalaJSON that only needs to use these flags for compilation (i.e. end users don't need to add these special flags?)

sirthias commented 7 years ago

Unfortunately its very significant (http4s/circe), but there are alternatives

Provide the current implementation just for scala 2.10.x. Means more code to maintain (although >not a real issue with a library that is meant to rarely change) and use the case class for 2.11.x/2.12.x

Just use a case class for the entire codebase and present the Scala 2.10 users with a big > warning label that they can access the constructor themselves

To be honest, even though not great, I'd prefer the first of your listed alternatives over the manual case class way. Paying forever into the future with the drawbacks just for Scala 2.10 support now doesn't appear like a good tradeoff to me.

So how about splitting out a scala210 branch with a manual case class solution for JNumber and a proper case class for JNumber for everyone else?

Also with these compiler flags that are needed for Scala 2.11.x, I assume its ScalaJSON that only needs to use these flags for compilation (i.e. end users don't need to add these special flags?)

Yes, that is also my understanding. This flag only affects the compiler frontend.

mdedetrich commented 7 years ago

@sirthias Sounds like a separate branch for 2.10.x support is the way, will work on it tonight

mdedetrich commented 7 years ago

@sirthias @ktoso @Ichoran New PR is up. Lot of stuff has changed, have removed underlying (its now pointless) and there is a build that is specific only for Scala 2.10.

@sirthias I have provided an apply for JNumber("1"): Option[JNumber] but it only works for JVM Scala 2.11.x and Scala 2.12.x (doesn't work for Scala.js, nor does it work for Scala 2.10.x). For this reason, there is a JNumber.fromString method that works on all platforms and all Scala versions

mdedetrich commented 7 years ago

@dwijnand @eed3si9n @ktoso @sirthias @Ichoran @gmethvin I am going to release 1.0.0-M4 now and merge this into master.

@sirthias I have created an issue at https://github.com/mdedetrich/scalajson/issues/36 for your concern regarding numericStringEquals if you want to talk about it there.

Also the name based extractor optimizations no longer apply, named based extractors only came into Scala 2.11.x, so its irrelevant in the scala210 branch. In the normal branch we use standard case classes which should automatically generate named based extractors by default

mdedetrich / scalajson

Turns the standard AST `JNumber` into a safe constructor #30

Codecov Report