Skip to content

Solve update kotlintest problems #1255

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 27 commits into from
Jan 24, 2019
Merged

Solve update kotlintest problems #1255

merged 27 commits into from
Jan 24, 2019

Conversation

AdrianRaFo
Copy link
Contributor

Fix the problem because the laws are not running.
Also fix some deprecations and a bad merge from master

@AdrianRaFo AdrianRaFo requested a review from a team January 17, 2019 17:29
nomisRev
nomisRev requested changes Jan 17, 2019
View reviewed changes
nomisRev
nomisRev approved these changes Jan 18, 2019
View reviewed changes
@nomisRev
Copy link
Member

@raulraja @pakoito @AdrianRaFo RxJava & Reactor instances should be fixed and the new Bracket law passes for them.

That just leaves DeferredK. Had a small discussion with @raulraja yesterday about this. The best we could come up with was removing the current DeferredK impl in favor of an interface Deferred<A> or interface Job<A> impl based of IO. Thoughts?

@raulraja
Copy link
Member

@nomisRev I think the IO strategy bundling dispatchers is the best one we have so far. Are you suggesting we merge this as is? and continue in a different PR for that?

@nomisRev
Copy link
Member

nomisRev commented Jan 22, 2019 •
edited
Loading

@raulraja well, if we're going with that strategy than DeferredK becomes deprecated? Which I would be in favor of given how much time I/we have spent trying to make it work, and it seems I keep failing :D

So if we're going to provide an implementation for interface Deferred<A> based off IO than I don't really see the point doing it in this PR? No, point in blocking this PR for issues we already have in master and even released. Not running laws on master is a bigger issue IMO.

@raulraja
Copy link
Member

Merge at will

pakoito
pakoito reviewed Jan 22, 2019
View reviewed changes
pakoito
pakoito reviewed Jan 22, 2019
View reviewed changes
modules/core/arrow-test/src/main/kotlin/arrow/test/UnitSpec.kt Outdated
@@ -11,28 +10,19 @@ import io.kotlintest.specs.AbstractStringSpec
*/
abstract class UnitSpec : AbstractStringSpec() {

private val lawTestCases = mutableListOf<TestCase>()

fun testLaws(vararg laws: List<Law>): List<TestCase> {
Copy link
Member

Choose a reason for hiding this comment

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

Can this be an expression body like

fun testLaws(vararg laws: List<Law>): List<TestCase> = laws.flatMap { ...

nomisRev
nomisRev requested changes Jan 23, 2019
View reviewed changes
Copy link
Member

@nomisRev nomisRev left a comment

Choose a reason for hiding this comment

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

I am removing my approval since I've done too much work on this PR myself.

I'll approve it once there is approval is a consensus from others.

@AdrianRaFo
Merge remote-tracking branch 'origin/arf-solve-update-kotlintest-prob…
27dcec9 
…lems' into arf-solve-update-kotlintest-problems
@AdrianRaFo
Copy link
Contributor Author

@pakoito comments addressed.
Thanks @nomisRev for your help

@raulraja
Copy link
Member

@AdrianRaFo https://api.travis-ci.org/v3/job/483637513/log.txt

@nomisRev
Copy link
Member

nomisRev commented Jan 24, 2019 •
edited
Loading

@pakoito could you take a look at the Rx fixes for Bracket?

Edit: did it again. I'm going to stop replying on my phone I keep closing PRs by accident..

@nomisRev nomisRev closed this Jan 24, 2019
@nomisRev nomisRev reopened this Jan 24, 2019
raulraja
raulraja approved these changes Jan 24, 2019
View reviewed changes
Copy link
Member

@raulraja raulraja left a comment

Choose a reason for hiding this comment

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

👏

@raulraja
Copy link
Member

@nomisRev ready to merge when you and @pakoito are

nomisRev
nomisRev approved these changes Jan 24, 2019
View reviewed changes
Copy link
Member

@nomisRev nomisRev left a comment

Choose a reason for hiding this comment

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

@pakoito could you do a post mortem PR review? I am going to merge already so we have laws running on master again.

Most specific for the Rx/Reactor Bracket bug fix.
ObservableK: https://github.com/arrow-kt/arrow/pull/1255/files#diff-dfc12f9d2453890aa332bed571a19f0a
SingleK: https://github.com/arrow-kt/arrow/pull/1255/files#diff-89d6c0960f46eac3d6f80d0f4ba14667
MaybeK: https://github.com/arrow-kt/arrow/pull/1255/files#diff-58109fc55101cf74a70000646921a14f
FlowableK: https://github.com/arrow-kt/arrow/pull/1255/files#diff-dcafde87192177e1924b0dca1510d9f0

MonoK: https://github.com/arrow-kt/arrow/pull/1255/files#diff-55a0cc4b7999477cdba28ae0cc9c8731
FluxK: https://github.com/arrow-kt/arrow/pull/1255/files#diff-d82a687b8b4cee7c8480040c8411d416

@nomisRev nomisRev merged commit 43e732e into master Jan 24, 2019
@nomisRev nomisRev deleted the arf-solve-update-kotlintest-problems branch January 24, 2019 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pakoito pakoito pakoito left review comments

@raulraja raulraja raulraja approved these changes

@nomisRev nomisRev nomisRev approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AdrianRaFo @nomisRev @raulraja @pakoito