Skip to content
This repository has been archived by the owner on Feb 24, 2021. It is now read-only.

Deprecate arrow.fx.coroutines.Duration #338

Closed
wants to merge 8 commits into from
Closed

Conversation

i-walker
Copy link
Member

@i-walker i-walker commented Dec 16, 2020

Status

On HOLD

Description

Deprecate and remove usages of arrow's Duration from all API's besides arrow.fx.Timer and other usages within arrow.fx, as it will be deprecated and removed soon.

  • Instead kotlin.time.Duration is used.
    In addition, this PR's centralizes some common build settings in
    • project("arrow-fx-coroutines"),
    • project("arrow-fx-coroutines-kotlinx-coroutines"),
    • project("arrow-fx-coroutines-test"),
    • project("arrow-fx-coroutines-stream"),
    • project("arrow-fx-stm")

Todos

  • Tests
  • Documentation

# Conflicts:
#	arrow-fx-coroutines/src/test/kotlin/arrow/fx/coroutines/BracketCaseTest.kt
#	arrow-fx-coroutines/src/test/kotlin/arrow/fx/coroutines/TimerTest.kt
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.

The added function for kotlin.time.Duration in timer.kt should be removed. We don't want to add new code there since KotlinX already covers all of these APIs. Does this remove all references to sleep/timeOutOrNull from the documentation and code?

I don't understand the diff from arrow-fx-coroutines/src/test/kotlin/arrow/fx/coroutines/TimerTest.kt. Is it re-added as an empty file?

Thanks for taking the time to do some clean-up!

* }
* ```
**/
suspend fun <A> timeOutOrNull(duration: kotlin.time.Duration, fa: suspend () -> A): A? =
Copy link
Member

Choose a reason for hiding this comment

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

Users should migrate to KotlinX withTimeoutOrNull.

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 will remove that :D

@@ -36,7 +37,7 @@ class EvalOnTests : ArrowFxSpec(spec = {

"evalOn on the same context doesn't dispatch" {
suspend fun onSameContext(): String =
evalOn(ComputationPool) {
withContext(ComputationPool) {
Copy link
Member

Choose a reason for hiding this comment

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

This is just testing code from another library now.
If we want to make such a change we instead just remove EvalOnTests from Arrow Fx Coroutines since evalOn now only delegates to withContext.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok I will do that

package arrow.fx.coroutines

import java.util.concurrent.TimeUnit

// TODO replace with kotlin.time
@Deprecated(
"Redundant since 0.12.0 will provide automatic integration with KotlinX",
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to provide a clearer message here. Duration is Deprecated in favor of kotlin.time.Duration, and KotlinX offers all APIs we offer in timer.kt for both kotlin.time.Duration and milliseconds: Long so APIs are available to the user with and without optining in for kotlin.time.

But that's a bit too long for a deprecated message I guess 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I was also unsure, what message would make sense in this case.
But I will go for yours, regardless 😄

@@ -0,0 +1,190 @@
#!/bin/bash
Copy link
Member

Choose a reason for hiding this comment

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

What is this file doing? Is this change needed? cc\ @rachelcarmena

Copy link
Member Author

Choose a reason for hiding this comment

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

No it is not needed, I wanted to execute the buildArrowDoc gradle task and it couldn't find that file from my local instance. I will remove that.

Still I couldn't execute the gradle task.
Do you know how I can perform the Ank checks?

Copy link
Member

Choose a reason for hiding this comment

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

There is a buildArrowDoc task available for me when checking out Arrow Fx 🤔 I think it relies on the other project being available locally, so ./gradlew gitPullAll. Not 100% sure how that works. It's mostly easier to just run Ank on CI and fix all the issues that it raises 😅
That's why I made Ank work with Validated so long ago 😂

Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com>
@i-walker i-walker mentioned this pull request Dec 23, 2020
3 tasks
@i-walker
Copy link
Member Author

This PR's is getting too big, so I am breaking it into multiple sub PR's to show some of the Error's I am running into

@i-walker i-walker closed this Dec 23, 2020
@i-walker i-walker deleted the is-deprecate-duration branch January 5, 2021 11:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants