mdedetrich / scalajson

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

Give unstable.JObject/JArray useful toStrings #46

Closed dwijnand closed 6 years ago

dwijnand commented 6 years ago

Testing for both JVM and JS. Note the results are not the same.

I think they should have the same result, but I'll leave that as a stretch task for someone else.

Fixes #39

codecov-io commented 6 years ago

Codecov Report

Merging #46 into master will decrease coverage by 0.04%. The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #46      +/-   ##
==========================================
- Coverage   16.29%   16.25%   -0.05%     
==========================================
  Files           5        5              
  Lines         798      800       +2     
  Branches      229      226       -3     
==========================================
  Hits          130      130              
- Misses        668      670       +2
Impacted Files Coverage Δ
...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 6262c4e...d2dd3b2. Read the comment docs.

dwijnand commented 6 years ago

This is passing Travis CI now. It's failing Codecov, but it looks to be wrong.

So this is ready for review.

dwijnand commented 6 years ago

I've pushed a commit switching to Arrays.toString. WDYT?

mdedetrich commented 6 years ago

@dwijnand Looking really good, I will have a proper look once I get back from holidays and if all good I will merge it!

dwijnand commented 6 years ago

Is there any more work that I can do on my end to help this along?

mdedetrich commented 6 years ago

@dwijnand Totally forgot about it, had christmas + new years and my bday. Merging it now, thanks for the work!

dwijnand commented 6 years ago

Belated Happy Birthday, then! Thanks for the merge.