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.

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

Add TraverseFilter instance for Queue. #3103

Merged
merged 1 commit into from
Nov 6, 2019
Merged

Add TraverseFilter instance for Queue. #3103

merged 1 commit into from
Nov 6, 2019

Conversation

takayahilton
Copy link
Contributor

No description provided.

@codecov-io
Copy link

codecov-io commented Oct 9, 2019 •
edited
Loading

Codecov Report

Merging #3103 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3103      +/-   ##
==========================================
- Coverage   93.52%   93.51%   -0.02%     
==========================================
  Files         368      368              
  Lines        6998     7011      +13     
  Branches      198      197       -1     
==========================================
+ Hits         6545     6556      +11     
- Misses        453      455       +2
Impacted Files Coverage Δ
core/src/main/scala/cats/instances/queue.scala 100% <100%> (ø) ⬆️
testkit/src/main/scala/cats/tests/Helpers.scala 96% <0%> (-4%) ⬇️
...cala/cats/kernel/instances/FunctionInstances.scala 96.96% <0%> (-3.04%) ⬇️

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 ca591fc...f546484. Read the comment docs.

kailuowang
kailuowang suggested changes Oct 9, 2019
View reviewed changes
Copy link
Contributor

@kailuowang kailuowang left a comment

Choose a reason for hiding this comment

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

Thanks! Now that Scala 2.11 was dropped from master, we no longer need binCompat traits.

LukaJCB
LukaJCB reviewed Oct 9, 2019
View reviewed changes
Copy link
Member

@LukaJCB LukaJCB left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks, this looks great! :)

Do we still need the bincompat traits now that we've dropped the binary compatability requirement on 2.11?

Edit: Oh, I'm late.

@takayahilton
Copy link
Contributor Author

takayahilton commented Oct 9, 2019 •
edited
Loading

@kailuowang @LukaJCB
I tried to add catsStdTraverseFilterForQueue to QueueInstances, but still Mima fails with ReversedMissingMethodProblem.
Could you tell me where should I add it?

@takayahilton takayahilton force-pushed the add-traverse-filter-instance-for-queue branch from a7f0097 to f546484 Compare October 11, 2019 06:26
takayahilton
takayahilton commented Oct 11, 2019
View reviewed changes
core/src/main/scala/cats/instances/queue.scala
@@ -152,4 +152,33 @@ trait QueueInstances extends cats.kernel.instances.QueueInstances {
def show(fa: Queue[A]): String =
fa.iterator.map(_.show).mkString("Queue(", ", ", ")")
}

implicit def catsStdTraverseFilterForQueue: TraverseFilter[Queue] = QueueInstances.catsStdTraverseFilterForQueue
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know why , but implicit val catsStdTraverseFilterForQueue breaks binary compatibility.

[error]  * abstract synthetic method cats$instances$QueueInstances$_setter_$catsStdTraverseFilterForQueue_=(cats.TraverseFilter)Unit in interface cats.instances.QueueInstances is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("cats.instances.QueueInstances.cats$instances$QueueInstances$_setter_$catsStdTraverseFilterForQueue_=")
[error]  * abstract method catsStdTraverseFilterForQueue()cats.TraverseFilter in interface cats.instances.QueueInstances is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("cats.instances.QueueInstances.catsStdTraverseFilterForQueue")

takayahilton
takayahilton commented Oct 11, 2019
View reviewed changes
core/src/main/scala/cats/instances/queue.scala
}

private object QueueInstances {
private val catsStdTraverseFilterForQueue: TraverseFilter[Queue] = new TraverseFilter[Queue] {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just an instance cache.

Copy link
Contributor

@kailuowang kailuowang Oct 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I don't see this pattern as problematic but it's not very consistent with rest of the code base, can we just make the one in the trait a val instead of creating a new object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I get an error that

[error]  * abstract synthetic method cats$instances$QueueInstances$_setter_$cats$instances$QueueInstances$$_catsStdTraverseFilterForQueue_=(cats.TraverseFilter)Unit in interface cats.instances.QueueInstances is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("cats.instances.QueueInstances.cats$instances$QueueInstances$_setter_$cats$instances$QueueInstances$$_catsStdTraverseFilterForQueue_=")
[error]  * abstract synthetic method cats$instances$QueueInstances$$_catsStdTraverseFilterForQueue()cats.TraverseFilter in interface cats.instances.QueueInstances is present only in current version

Copy link
Contributor

Choose a reason for hiding this comment

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

how did you run the check? did you get that error from sbt validateBC?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I ran sbt validateBC.

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding a val to a trait will break bincompat on all Scala versions so far because of the synthetic setter that's necessary because of the way trait initialization works. The workaround here looks reasonable to me.

kailuowang
kailuowang previously approved these changes Oct 18, 2019
View reviewed changes
@kailuowang kailuowang dismissed their stale review October 19, 2019 00:14

missed one thing

@travisbrown travisbrown added this to the 2.1.0-RC1 milestone Nov 5, 2019
@LukaJCB LukaJCB merged commit 0aaa637 into typelevel:master Nov 6, 2019
@takayahilton takayahilton deleted the add-traverse-filter-instance-for-queue branch November 6, 2019 11:06
@travisbrown travisbrown mentioned this pull request Nov 13, 2019
Closed
22 tasks
gagandeepkalra added a commit to gagandeepkalra/cats that referenced this pull request Feb 11, 2020
@gagandeepkalra
backported typelevel#3103 Add traverseFilter instances for Queue
c541f77
travisbrown pushed a commit that referenced this pull request Mar 11, 2020
@gagandeepkalra
backported #3103 Add traverseFilter instances for Queue (#3292)
f82cd0d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kailuowang kailuowang kailuowang left review comments

@travisbrown travisbrown travisbrown approved these changes

@LukaJCB LukaJCB LukaJCB approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
2.1.0-RC1
Development

Successfully merging this pull request may close these issues.
5 participants
@takayahilton @codecov-io @kailuowang @travisbrown @LukaJCB