Skip to content

Deprecate gather, gatherN, gatherUnordered in favor of parSequence, parSequenceN, parSequenceUnordered - #1145

Merged
Avasil merged 2 commits into
monix:masterfrom
jozic:#1130
Mar 23, 2020
Merged

Deprecate gather, gatherN, gatherUnordered in favor of parSequence, parSequenceN, parSequenceUnordered#1145
Avasil merged 2 commits into
monix:masterfrom
jozic:#1130

Conversation

@jozic

@jozic jozic commented Mar 15, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

addresses #1130

@jozic

jozic commented Mar 15, 2020

Copy link
Copy Markdown
Contributor Author

@Avasil @oleg-py @alexandru
please check this out, if this looks good, i can continue with the rest

@jozic jozic changed the title Deprecate gather, gatherN, gatherUnordered in favor of parTraverse, p… Deprecate gather, gatherN, gatherUnordered in favor of parSequence, p… Mar 15, 2020
@jozic jozic changed the title Deprecate gather, gatherN, gatherUnordered in favor of parSequence, p… Deprecate gather, gatherN, gatherUnordered in favor of parSequence, parSequenceN, parSequenceUnordered Mar 15, 2020
Avasil
Avasil reviewed Mar 22, 2020
View reviewed changes

@Avasil Avasil left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks @jozic - I think it looks great!

I added one comment about location of depreacted methods and we also have to make it compile / pass mimaReportBinaryIssues check:

  • We have benchmark module for a previous version as well so we have to either keep using gather or temporarily turn off its compilation in CI (at the top of build.sbt named as benchmarksPrev )
  • We need to add filters to MimaFilters for renamed objects with implementations. They are private so it's ok. It can be verified with evalJVM/mimaReportBinaryIssues

Comment thread monix-eval/shared/src/main/scala/monix/eval/Task.scala Outdated

/** Alias for [[parSequence]] */
@deprecated("Use parSequence", "3.2.0")
def gather[A, M[X] <: Iterable[X]](in: M[Task[A]])(implicit bf: BuildFrom[M[Task[A]], A, M[A]]): Task[M[A]] =

@Avasil Avasil Mar 22, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We should put deprecated methods to TaskDeprecated.Companion to clean normal API

There is also slightly different convention for description with

/**
   * DEPRECATED — please use XXX
   */

@jozic

jozic commented Mar 22, 2020

Copy link
Copy Markdown
Contributor Author

@Avasil thanks for feedback!
I've moved deprecated methods to TaskDeprecated.Companion and also added renamed classes to the mima filter
I've also tried to revert changes to benchmarks, but current version wasn't compilable due to deprecation errors, so I've disabled benchmarksPrev
CI seems happy now

Please check it out again

@Avasil

Avasil commented Mar 22, 2020

Copy link
Copy Markdown
Collaborator

@jozic Looks good, we can add the rest 👍

I think we can wait with monix.io docs update until 3.2.0 is released

@jozic

jozic commented Mar 23, 2020

Copy link
Copy Markdown
Contributor Author

@Avasil I've added second commit with changes for wander and friends
lmk if anything need to be changed

@Avasil
Avasil marked this pull request as ready for review March 23, 2020 08:32
Avasil
Avasil approved these changes Mar 23, 2020
View reviewed changes

@Avasil Avasil left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Awesome, thank you @jozic :)

@Avasil
Avasil merged commit a8b3cc4 into monix:master Mar 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Avasil Avasil Avasil 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.

2 participants

@jozic @Avasil