Fix Json.add and Json.merge operations #129

ambrusadrianzh · 2019-10-22T15:47:40Z

Fixes #118

Changes:

Json.add should return object with added/updated field
Json.merge should work as Map.plus
Json.toList should use Pair to Tuple2 function
Add test cases for add and merge operations

* Json.add should return object with added/updated field * Json.merge should work as Map.plus * Json.toList should use Pair to Tuple2 function * Add test cases for add and merge operations

AdrianRaFo

I rather think that, after this, both add and merge are the same and we should delete one of them.
Apart from that, I'm my opinion, both methods only works on JsObjects so one of both we should define the methods on each Json type or move them to JsObject. This second thing could be discussed on a different issue and done on a different PR if you like

helios-core/src/main/kotlin/helios/core/helios.kt

ambrusadrianzh · 2019-10-23T07:36:00Z

Agreed, add and merge should only work on JsObjects.

However, I'm not sure if add and merge serve the same purpose. With add you can add JsString, JsNumber etc. to a JsObject with a certain key.

The merge function should merge two JsObjects together. This way we can avoid the following confusing situtations:

val a = JsObject("weight" to JsFloat(61.4f))
val b = JsString("abc")

val c = a.merge(b) // c == b for no reason

If we constrain the merge function to JsObjects only, these situations will not able to happen. I'm happy to make these changes, if we can agree on these.

AdrianRaFo · 2019-10-23T07:45:21Z

As it is on this PR, merge serves the same purpose than add because if, continuing with your example, a.merge(b) doesn't do anything because b has to be a JsObject.
You can restrict that making merge to receive a JsObject directly and it would be the same as add with the insignificant difference that add just allow you to add new values one by one and merge would allow you to add them all at the same call.

AdrianRaFo · 2019-10-23T07:46:24Z

I agree to move them to JsObject but I would like the opinion of @nomisRev here too

ambrusadrianzh · 2019-10-28T09:12:47Z

@AdrianRaFo @nomisRev How should we proceed? 🤔

nomisRev

Very excited to see contributions to Helios! 👏👏👏

helios-core/src/main/kotlin/helios/core/helios.kt

* move add operation from Json to JsObject * add Monoid to Json.merge operation

ambrusadrianzh · 2019-12-02T13:07:04Z

Hey @nomisRev,

Sorry for the delay. Made the changes today, caught up with upstream and integrated the testing changes from arrow test. Please take a look at it, as I'm not sure I got it right.

helios-core/build.gradle

helios-core/src/main/kotlin/helios/core/helios.kt

AdrianRaFo · 2019-12-02T19:04:35Z

helios-core/src/main/kotlin/helios/core/monoid.kt

+
+fun JsObject.Companion.semigroup(): JsObjectSemigroup = object : JsObjectSemigroup {  }
+
+fun JsObject.Companion.monoid(): JsObjectMonoid = object : JsObjectMonoid {  }


I'm missing JsNull, JsDecimal, JsNumber, and JsBoolean, also maybe we can add a Monoid of Json matching over all these monoids

AdrianRaFo · 2019-12-02T19:06:15Z

helios-core/src/test/kotlin/helios/core/MonoidTest.kt

+                JsString.eq()
+            )
+        )
+    }


A tests for each monoid?

AdrianRaFo · 2019-12-02T19:06:46Z

helios-core/src/test/kotlin/helios/core/MonoidTest.kt

+        )
+
+        testLaws(
+            MonoidLaws.laws(


I guess Monoid laws also includes Semigroup laws

AdrianRaFo · 2019-12-02T19:08:24Z

helios-optics/src/test/kotlin/helios/optics/instances.kt

-        override fun Option<Json>.combine(b: Option<Json>) = flatMap { a -> b.map { a.merge(it) } }
-        override fun empty(): Option<Json> = None
-      }
-    ))


Why are we deleting the Lens law test?

AdrianRaFo · 2019-12-02T19:08:56Z

helios-core/src/test/kotlin/helios/core/JsonOperationTest.kt

-            }
-        }
-    }
-}


This file should be updated, not deleted

nomisRev · 2019-12-03T10:02:52Z

@ambrusadrianzh no worries! Thank you for the contributions! I saw @AdrianRaFo already checked out your PR, I can give it a look next week. This week is a little busy with travelling and KotlinConf ;)

Fix Json.add and Json.merge operations

c9ab7fb

* Json.add should return object with added/updated field * Json.merge should work as Map.plus * Json.toList should use Pair to Tuple2 function * Add test cases for add and merge operations

calvellido requested review from AdrianRaFo and nomisRev October 23, 2019 06:37

AdrianRaFo reviewed Oct 23, 2019

View reviewed changes

helios-core/src/main/kotlin/helios/core/helios.kt Outdated Show resolved Hide resolved

nomisRev requested changes Nov 8, 2019

View reviewed changes

helios-core/src/main/kotlin/helios/core/helios.kt Outdated Show resolved Hide resolved

helios-core/src/main/kotlin/helios/core/helios.kt Outdated Show resolved Hide resolved

ambrusadrianz added 2 commits December 2, 2019 09:43

Address PR comments

e012fc0

* move add operation from Json to JsObject * add Monoid to Json.merge operation

Merge branch 'master' into fix/add_merge_operations

974702e

Delete manual Monoid and Semigroup implementations

b04d554

AdrianRaFo requested changes Dec 2, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Json.add and Json.merge operations #129

Fix Json.add and Json.merge operations #129

ambrusadrianzh commented Oct 22, 2019

AdrianRaFo left a comment •

edited

ambrusadrianzh commented Oct 23, 2019

AdrianRaFo commented Oct 23, 2019

AdrianRaFo commented Oct 23, 2019 •

edited

ambrusadrianzh commented Oct 28, 2019

nomisRev left a comment

ambrusadrianzh commented Dec 2, 2019

AdrianRaFo Dec 2, 2019

AdrianRaFo Dec 2, 2019

AdrianRaFo Dec 2, 2019

AdrianRaFo Dec 2, 2019

AdrianRaFo Dec 2, 2019

nomisRev commented Dec 3, 2019


		fun JsObject.Companion.semigroup(): JsObjectSemigroup = object : JsObjectSemigroup { }

		fun JsObject.Companion.monoid(): JsObjectMonoid = object : JsObjectMonoid { }

Fix Json.add and Json.merge operations #129

Are you sure you want to change the base?

Fix Json.add and Json.merge operations #129

Conversation

ambrusadrianzh commented Oct 22, 2019

AdrianRaFo left a comment • edited

Choose a reason for hiding this comment

ambrusadrianzh commented Oct 23, 2019

AdrianRaFo commented Oct 23, 2019

AdrianRaFo commented Oct 23, 2019 • edited

ambrusadrianzh commented Oct 28, 2019

nomisRev left a comment

Choose a reason for hiding this comment

ambrusadrianzh commented Dec 2, 2019

AdrianRaFo Dec 2, 2019

Choose a reason for hiding this comment

AdrianRaFo Dec 2, 2019

Choose a reason for hiding this comment

AdrianRaFo Dec 2, 2019

Choose a reason for hiding this comment

AdrianRaFo Dec 2, 2019

Choose a reason for hiding this comment

AdrianRaFo Dec 2, 2019

Choose a reason for hiding this comment

nomisRev commented Dec 3, 2019

AdrianRaFo left a comment •

edited

AdrianRaFo commented Oct 23, 2019 •

edited