mdedetrich / scalajson

ScalaJSON - JSON for Scala, currently contains minimal AST
BSD 3-Clause "New" or "Revised" License
55 stars 10 forks source link

Allow you to supply a custom collection on .toStandard #51

Open mdedetrich opened 6 years ago

mdedetrich commented 6 years ago

As discussed on the last scala center meeting, there was a request to add the feature where if you call .toStandard on an unsafe JSON AST, you have the ability to specify the type of Map so you can control whether you will lose key ordering and/or you will lose effectively constant lookup time.

This implementation uses an implicit canBuildFrom on the .toStandard method to construct the standard AST. The default collection is an immutable Map, but you can also use a ListMap or a LinkedMap by simply providing the type, i.e. you can do toStandard or toStandard[ListMap] or toStandard[LinkedMap]. This means there are no breaking source changes, you can use .toStandard just like you did before (and it will use the Map implementation just as before) but you can specify a custom type if you want.

Theoretically there shouldn't be any performance issue but I will have to run benchmarks to closely analyse. There are also alternate ways you can solve this problem, i.e. you can choose to put the CBF[C[A, B] <: Map[A, B]] directly inside the JSON AST sealed abstract class. This means that on construction the JSON AST will contain the refined type of whatever Map type you constructed it with, on the other hand you will have a downside of making the AST more complex (with this method, its only the .toStandard method which is changed, otherwise the AST remains completely the same).

It should also be discussed whether or not such a feature is worth the complexity it adds (either this current PR or other proposals).

Also ran scalafmt on test source so there is some noise in the PR.

@jvican @Ichoran

mdedetrich commented 6 years ago

@SethTisue This PR is failing due to scalac throwing some position error for scala-2.13.0-M3, any ideas? You can see the run here https://travis-ci.org/mdedetrich/scalajson/jobs/347380642

SethTisue commented 6 years ago

looks like https://github.com/scala/bug/issues/10706, believed fixed for M4. workaround: disable -Yrangepos

codecov-io commented 6 years ago

Codecov Report

Merging #51 into master will not change coverage. The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #51   +/-   ##
=======================================
  Coverage   16.12%   16.12%           
=======================================
  Files           5        5           
  Lines         806      806           
  Branches      241      242    +1     
=======================================
  Hits          130      130           
  Misses        676      676
Impacted Files Coverage Δ
...s/src/main/scala/scalajson/ast/unsafe/JValue.scala 0% <0%> (ø) :arrow_up:
...m/src/main/scala/scalajson/ast/unsafe/JValue.scala 0% <0%> (ø) :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 b66adb2...84f232f. Read the comment docs.

mdedetrich commented 6 years ago

Okay thanks, I will leave the PR open so people can comment on it in the next Scala Center meeting

jvican commented 6 years ago

Do you mean the Scala Platform meeting? :P

mdedetrich commented 6 years ago

Do you mean the Scala Platform meeting? :P

Yes indeed, got them mixed up!