Add wrapper for SortedMap #451

Cotel · 2017-11-07T19:07:33Z

Attempt to solve #205

It is almost the same code as in #310 except this time we are using SortedMap.

I'm trying to change the tests to be more like ListKW ones so this is still WIP 😄

pakoito · 2017-11-07T21:48:50Z

kategory-core/src/main/kotlin/kategory/data/SortedMapKW.kt

+    fun <G, B> traverse(f: (A) -> HK<G, B>, GA: Applicative<G>): HK<G, SortedMapKW<K, B>> =
+        Foldable.iterateRight(this.map.iterator(), Eval.always { GA.pure(sortedMapOf<K, B>().k()) })({
+            kv, lbuf ->
+            GA.map2Eval(f(kv.value), lbuf) { (sortedMapOf(kv.key to it.a).k() + it.b).toSortedMap().k() }


If we're going to sort it afterwards, is there any perf gain from using (mapOf(kv.key to it.a).k() + it.b) here instead?

I don't really know, but there's no need for the first map to be sorted if we are sorting it right after as you say. I'm changing this 👍

pakoito · 2017-11-07T21:50:10Z

kategory-core/src/main/kotlin/kategory/data/SortedMapKW.kt

+
+fun <K: Comparable<K>, A> SortedMap<K, A>.k(): SortedMapKW<K, A> = SortedMapKW(this)
+
+fun <K: Comparable<K>, A> Option<Tuple2<K, A>>.k(): SortedMapKW<K, A> = when (this) {


I'd suggest using fold instead of when.

raulraja · 2017-11-07T21:52:54Z

@Cotel just wanna say so far it's looking awesome, thanks so much for your contribution!

raulraja · 2017-11-07T21:54:31Z

kategory-core/src/main/kotlin/kategory/data/SortedMapKW.kt

+import java.util.*
+
+@higherkind
+data class SortedMapKW<K: Comparable<K>, A>(val map: SortedMap<K, A>) : SortedMapKWKind<K, A>, SortedMap<K, A> by map {


If we are going to follow here the K, V instead of A, B we should rename A for V so it denotes we refer to the value associated to a key otherwise use A, B which is the Kategory convention for applied types that are not in higher kind position.

I didn't know A, B was the convention. I will change it 👌

pakoito · 2017-11-07T21:54:40Z

kategory-core/src/test/kotlin/kategory/data/SortedMapKWTest.kt

+
+        val monoid = SortedMapKW.monoid<String, Int>(SG)
+
+        "Monoid Laws: identity" {


This may be wrong in Map too. This should be tested using MonoidLaws.laws rather than in the file.

@pakoito we only have MonoidkLaws :(

We should then add Monoid and Semigroup laws before merging this. @Cotel wanna take care of those in this PR as well?

I can try 💪

pakoito · 2017-11-07T23:28:20Z

kategory-core/src/test/kotlin/kategory/data/SortedMapKWTest.kt

+    }
+
+
+}


NIT: newline at the end of the file

pakoito · 2017-11-07T23:29:18Z

@Cotel Weeee, thank you for the PR :D

Cotel · 2017-11-08T09:15:15Z

Thank you for your feedback. I hope this to be the first of many contributions 😃

codecov-io · 2017-11-08T09:52:36Z

Codecov Report

Merging #451 into master will increase coverage by 0.04%.
The diff coverage is 40%.

@@             Coverage Diff              @@
##             master     #451      +/-   ##
============================================
+ Coverage     36.07%   36.11%   +0.04%     
- Complexity      320      327       +7     
============================================
  Files           174      178       +4     
  Lines          4846     4901      +55     
  Branches        523      523              
============================================
+ Hits           1748     1770      +22     
- Misses         2950     2982      +32     
- Partials        148      149       +1

Impacted Files	Coverage Δ	Complexity Δ
...y-test/src/main/kotlin/kategory/laws/MonoidLaws.kt	`14.28% <14.28%> (ø)`	`1 <1> (?)`
...est/src/main/kotlin/kategory/laws/SemigroupLaws.kt	`33.33% <33.33%> (ø)`	`1 <1> (?)`
...-core/src/main/kotlin/kategory/data/SortedMapKW.kt	`37.83% <37.83%> (ø)`	`5 <5> (?)`
.../kotlin/kategory/instances/SortedMapKWInstances.kt	`75% <75%> (ø)`	`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 a88e4d1...715bf58. Read the comment docs.

pakoito

Approval for the PR. I won't merge until I get thumbs up for the new laws!

Cotel · 2017-11-13T13:03:21Z

I've added an implementation for MonoidLaws and SemigroupLaws. But I am still wondering if SortedMapKWTest should test MonoidKLaws and SemigroupKLaws too 🤔

raulraja · 2017-11-13T13:22:28Z

@Cotel SortedMap does not provide instances for MonoidK or SemigroupK so no need to test those. We need now a new issue ensuring all instances of Monoid and Semigroup provided by Kategory are being tested by the new laws. Awesome work!

Cotel added 2 commits November 7, 2017 13:59

First approach to implement SortedMapKW (tests in progress)

4bb0cd7

SortedMapKW tests passing

622c46e

pakoito reviewed Nov 7, 2017

View reviewed changes

raulraja reviewed Nov 7, 2017

View reviewed changes

pakoito reviewed Nov 7, 2017

View reviewed changes

kategory-core/src/test/kotlin/kategory/data/SortedMapKWTest.kt

}

}

Copy link

Member

pakoito Nov 7, 2017

Choose a reason for hiding this comment

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

NIT: newline at the end of the file

Using mapOf instead of sortedMapOf

4700dfc

Cotel and others added 4 commits November 8, 2017 11:09

Changing Type Parameters to follow Kategory's convention

34a52e0

Merge branch 'master' into master

bb423ed

Using fold instead of when

9e0a761

Merge branch 'master' into master

6cf6b73

pakoito approved these changes Nov 11, 2017

View reviewed changes

Adding MonoidLaws and SemigroupLaws

715bf58

raulraja merged commit 185cf36 into arrow-kt:master Nov 13, 2017

raulraja mentioned this pull request Nov 13, 2017

Ensure all instances of Semigroup and Monoid are tested with laws #460

Closed

pakoito mentioned this pull request Feb 8, 2018

Create Wrapper class for SortedMap #205

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add wrapper for SortedMap #451

Add wrapper for SortedMap #451

Cotel commented Nov 7, 2017 •

edited

Loading

pakoito Nov 7, 2017

Cotel Nov 8, 2017

pakoito Nov 7, 2017

raulraja commented Nov 7, 2017

raulraja Nov 7, 2017

Cotel Nov 8, 2017 •

edited

Loading

pakoito Nov 7, 2017

victorg1991 Nov 8, 2017

raulraja Nov 9, 2017

Cotel Nov 10, 2017

pakoito Nov 7, 2017

pakoito commented Nov 7, 2017

Cotel commented Nov 8, 2017

codecov-io commented Nov 8, 2017 •

edited

Loading

pakoito left a comment

Cotel commented Nov 13, 2017

raulraja commented Nov 13, 2017


		fun <K: Comparable<K>, A> SortedMap<K, A>.k(): SortedMapKW<K, A> = SortedMapKW(this)

		fun <K: Comparable<K>, A> Option<Tuple2<K, A>>.k(): SortedMapKW<K, A> = when (this) {


		val monoid = SortedMapKW.monoid<String, Int>(SG)

		"Monoid Laws: identity" {

Add wrapper for SortedMap #451

Add wrapper for SortedMap #451

Conversation

Cotel commented Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raulraja commented Nov 7, 2017

Choose a reason for hiding this comment

Cotel Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pakoito commented Nov 7, 2017

Cotel commented Nov 8, 2017

codecov-io commented Nov 8, 2017 • edited Loading

Codecov Report

pakoito left a comment

Choose a reason for hiding this comment

Cotel commented Nov 13, 2017

raulraja commented Nov 13, 2017

Cotel commented Nov 7, 2017 •

edited

Loading

Cotel Nov 8, 2017 •

edited

Loading

codecov-io commented Nov 8, 2017 •

edited

Loading