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

WIP. Reorganize all instances in a single file per datatype #130

Merged
merged 29 commits into from
Jul 18, 2017

Conversation

raulraja
Copy link
Member

WIP, Do not merge.

This PR reorganizes all instances to be in a single file for all the instances of a given datatype.
Instances that can be materialized without args are aggregated as part of the dataype's companion.
Others such as OptionMonoid are exposed in the companion's function as functions that you can invoke with the proper params to retrieve the instance.

@codecov-io
Copy link

codecov-io commented Jul 17, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@0cbe5ca). Click here to learn what that means.
The diff coverage is 43.64%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #130   +/-   ##
=========================================
  Coverage          ?   53.69%           
  Complexity        ?      160           
=========================================
  Files             ?       66           
  Lines             ?     1408           
  Branches          ?      174           
=========================================
  Hits              ?      756           
  Misses            ?      581           
  Partials          ?       71
Impacted Files Coverage Δ Complexity Δ
kategory/src/main/kotlin/kategory/predef.kt 25% <ø> (ø) 0 <0> (?)
kategory/src/main/kotlin/kategory/data/Composed.kt 80% <ø> (ø) 0 <0> (?)
...ain/kotlin/kategory/instances/CoYonedaInstances.kt 100% <ø> (ø) 0 <0> (?)
...rc/main/kotlin/kategory/instances/EvalInstances.kt 57.14% <ø> (ø) 0 <0> (?)
...y/src/main/kotlin/kategory/typeclasses/Traverse.kt 0% <ø> (ø) 0 <0> (?)
.../main/kotlin/kategory/instances/CofreeInstances.kt 50% <ø> (ø) 0 <0> (?)
kategory/src/main/kotlin/kategory/data/Ior.kt 40% <0%> (ø) 16 <0> (?)
kategory/src/main/kotlin/kategory/arrow/Kleisli.kt 55.55% <0%> (ø) 8 <0> (?)
kategory/src/main/kotlin/kategory/data/WriterT.kt 44.44% <0%> (ø) 7 <0> (?)
...tegory/src/main/kotlin/kategory/arrow/Function0.kt 58.82% <0%> (ø) 1 <0> (?)
... and 33 more

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 0cbe5ca...001fe14. Read the comment docs.

@@ -18,6 +18,16 @@ class StateT<F, S, A>(
companion object {
inline operator fun <reified F, S, A> invoke(run: StateTFunKind<F, S, A>, MF: Monad<F> = monad<F>()): StateT<F, S, A> =
StateT(MF, run)

fun <F, S> instances(MF: Monad<F>): StateTInstances<F, S> = object : StateTInstances<F, S> {
Copy link
Member

Choose a reason for hiding this comment

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

This could use inline + reify + default value for monad.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll do a pass on all those methods before merging

@@ -8,7 +8,10 @@ import org.junit.runner.RunWith
class EvalTest : UnitSpec() {
init {

testLaws(MonadLaws.laws(Eval, EvalEq()))
testLaws(MonadLaws.laws(Eval, object : Eq<HK<Eval.F, Int>> {
Copy link
Member

Choose a reason for hiding this comment

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

The Eq instance for Eval should be valid even for the main library. Was this a merge conflict, or is there something wrong with the previous one?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was fixed to Int, that would just work on tests.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha! Also, woops :P Could you please make a generic version instead that wraps another Eq?

Copy link
Member

@pakoito pakoito left a comment

Choose a reason for hiding this comment

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

Good job with the cleanup! It'll make it much easier for newcomers.

package kategory

class IOEq : Eq<HK<IO.F, Int>> {
override fun eqv(a: HK<IO.F, Int>, b: HK<IO.F, Int>): Boolean =
Copy link
Member

Choose a reason for hiding this comment

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

My god, I did all of them wrong. Shame on me.

Copy link
Member

@pakoito pakoito left a comment

Choose a reason for hiding this comment

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

I've been following the per-commit notifications all night. Massive job, great for consistence :D

So far only two requests before merging:

  • Add instance() to all companion objects regardless of parametrization. Just for consistence, as they contain monad(), traverse(), etc...
  • Make a pass on all parametrized companion object functions to become inlined with default values

I'll do a pass on all the Eq I did wrong the first time in another ticket.

EitherT.pure<Id.F, Int, Either<Int, Int>>(Either.Right(b * a))
}
}.ev().ev()
Copy link
Member

Choose a reason for hiding this comment

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

These two ev() may be causing an exception and why Travis stopped working after the previous test.

kategory.EitherTTest > EitherTTest.EitherTMonad#flatMap should be consistent with EitherT#flatMap PASSED
:kategory:test FAILED 

Seems like something for the Kotlin team.

Copy link
Member

@pakoito pakoito left a comment

Choose a reason for hiding this comment

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

Just one question

@@ -10,29 +10,29 @@ data class WriterT<F, W, A>(val MF: Monad<F>, val value: HK<F, Tuple2<W, A>>) :
class F private constructor()

companion object {
inline fun <reified F, reified W, A> pure(a: A, MM: Monoid<W> = monoid(), MF: Monad<F> = monad()) =
inline fun <reified F, reified W, A> pure(a: A, MM: Monoid<W> = monoid(), MF: Monad<F> = kategory.monad()) =
Copy link
Member

Choose a reason for hiding this comment

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

Why is the disambiguation needed here and not on the other instances?

Copy link
Member Author

Choose a reason for hiding this comment

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

because providing default args makes the monad() method ambiguous with the one in kategory. For other types where different args are required they are not necesary because is able to determine if it comes from kategory or the same companion instance.

@raulraja raulraja merged commit f3f5836 into master Jul 18, 2017
@raulraja raulraja deleted the rr-reorg-instances branch July 18, 2017 21:49
rachelcarmena added a commit that referenced this pull request Feb 24, 2021
* 0.11.0/bio (#1975)

* BIO - Part 1 (#1851)

* Add E param

* Fix imports & types

* Add RaiseError case

* Refactor IO name

* [BIO]: include `E` into IOFrame & IORunLoop (#1859)

* Rewrite IOFrame

* Update IORunLoop & fix resulting combinators

* Move public code to IO.kt

* Refactor Right to Success

* IO: IOConnection & IOBracket (#1870)

* Update ForwardCancelable

* Update IOBracket

* Remove KindConnection and refactor into IOConnection

* IO Concurrency combinators & Green Build (#1892)

* BIO - Part 1 (#1851)

* Add E param

* Fix imports & types

* Add RaiseError case

* Refactor IO name

* [BIO]: include `E` into IOFrame & IORunLoop (#1859)

* Rewrite IOFrame

* Update IORunLoop & fix resulting combinators

* Move public code to IO.kt

* Refactor Right to Success

* IO: IOConnection & IOBracket (#1870)

* Update ForwardCancelable

* Update IOBracket

* Remove KindConnection and refactor into IOConnection

* Rewrite UnsafePromise

* Rewrite ParMap2

* Update IOParMap3

* Update IORacePair

* Update IORaceTriple

* Fix tests

* Fix KIO benchmarks

* Fix imports Ank

* Fix IOExample

* Fix unused imports

* Fix imports benchmarks

* Fix arrow-integrations-retrofit-adapter

* Fix FpToTheMax & typeclasses example

* KtLintFormat

* Fix IOTest

* Fix more tests

* Fix merge conflicts

* KtLintFormat

* Fix MaybeK bracket and use it for QueueTest

* Throw proper exception in impossible cases

* Revert removing runAsync

* Fix build

* KtLintFormat

* Temporary solution for DSL receiver conflict overloads (#1994)

* Add fix and test (#2013)

* More meaningful functions mapping Either to BIO (#2002) (#2003)

* Add missing test and fix merge review changes (#2016)

* Change cancel wording to have 2 Ls for consistency (#2055)

* Arrow test conflict after merge

* Fix benchmarks

* Fix build

* Fix some errors in tests and code

Fix L spelling of cancellable

* Conf: separate 0.11.0 version

* Merge fixes

* Merge fixes

* Fix package for eq (#135)

* Fix doc generation and validation failures (#132)

Co-authored-by: nomisRev <vergauwen.simon@gmail.com>
Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com>

* After release 0.10.5: just considering one wip version (#133)

Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com>
Co-authored-by: Simon Vergauwen <vergauwen.simon@gmail.com>
Co-authored-by: Paco <pakoito@users.noreply.github.com>
Co-authored-by: Oskar Drozda <themppsplx@gmail.com>
Co-authored-by: Alberto Ballano <aballano@users.noreply.github.com>
Co-authored-by: raulraja <raulraja@users.noreply.github.com>
rachelcarmena pushed a commit that referenced this pull request Feb 24, 2021
* Fix asKoltin extension function to just working with fqName

* Fix :arrow-meta:ktlintMainSourceSetCheck

* Update arrow-meta/src/main/java/arrow/meta/encoder/jvm/StringTypeExtensions.kt

Co-authored-by: Alberto Ballano <aballano@users.noreply.github.com>

Co-authored-by: Alberto Ballano <aballano@users.noreply.github.com>
rachelcarmena pushed a commit that referenced this pull request Feb 24, 2021
* Revert "0.11.0-SNAPSHOT wip branch (#130)"

This reverts commit bd41883.

Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com>
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.

3 participants