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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mdedetrich
Copy link
Owner

@mdedetrich mdedetrich commented Feb 28, 2018

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
Copy link
Owner Author

mdedetrich commented Feb 28, 2018

@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
Copy link
Contributor

SethTisue commented Feb 28, 2018

looks like scala/bug#10706, believed fixed for M4. workaround: disable -Yrangepos

@codecov-io
Copy link

codecov-io commented Mar 2, 2018

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%> (ø) ⬆️
...m/src/main/scala/scalajson/ast/unsafe/JValue.scala 0% <0%> (ø) ⬆️

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.

Copy link
Collaborator

@jvican jvican left a comment

Choose a reason for hiding this comment

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

I like the approach and I think it makes the most sense. I don’t have anything to add rather than giving you a 👍!

@mdedetrich
Copy link
Owner Author

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

@jvican
Copy link
Collaborator

jvican commented Mar 7, 2018

Do you mean the Scala Platform meeting? :P

@mdedetrich
Copy link
Owner Author

Do you mean the Scala Platform meeting? :P

Yes indeed, got them mixed up!

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.

4 participants