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

fix: Added priority queue sink and source extending queue #3930

Conversation

neomaclin
Copy link
Contributor

@neomaclin neomaclin commented Jan 4, 2024

trying to continue the work #3733 by @lukecollier
@armanbilge

@neomaclin neomaclin force-pushed the fix/priority-queue-sink-source-use-queue-sink-source branch from 84c601b to 5b7f468 Compare January 4, 2024 18:37
@durban
Copy link
Contributor

durban commented Jan 5, 2024

It seems to me, that tryTakeN and tryOfferN has the same implementation in QueueSource/QueueSink. So why are we overriding?

@neomaclin
Copy link
Contributor Author

It seems to me, that tryTakeN and tryOfferN has the same implementation in QueueSource/QueueSink. So why are we overriding?

actually, come to think of it. they are actually identical in almost everyway. a simple extends without should just do., pushed

@neomaclin
Copy link
Contributor Author

neomaclin commented Jan 5, 2024

insteresting seeing the error in Mima check, maybe that is need of override?

@armanbilge
Copy link
Member

insteresting seeing the error in Mima check, maybe that is need of override?

yes, I think you can add an override call that delegates to the super so you do not need to repeat the implementation.

@neomaclin
Copy link
Contributor Author

neomaclin commented Jan 5, 2024

insteresting seeing the error in Mima check, maybe that is need of override?

yes, I think you can add an override call that delegates to the super so you do not need to repeat the implementation.

looks like Mima hates either way....(override with super call triggered the ReversedMissingMethodProblem)

@neomaclin
Copy link
Contributor Author

why DispatcherSpec failed is a bit beyound me...

@neomaclin neomaclin force-pushed the fix/priority-queue-sink-source-use-queue-sink-source branch from 452289a to fb356b7 Compare January 5, 2024 19:04
@neomaclin
Copy link
Contributor Author

It seems to me, that tryTakeN and tryOfferN has the same implementation in QueueSource/QueueSink. So why are we overriding?

I guess the attempts were telling us MiMa doesn't agree...

@armanbilge
Copy link
Member

looks like Mima hates either way....(override with super call triggered the ReversedMissingMethodProblem)

hmm. can you share the full MiMa error message? I don't understand why it's not happy.

@neomaclin
Copy link
Contributor Author

neomaclin commented Jan 6, 2024

looks like Mima hates either way....(override with super call triggered the ReversedMissingMethodProblem)

hmm. can you share the full MiMa error message? I don't understand why it's not happy.

ah. in order to fix the build, I have force pushed and squashed that commit (not sure how to find the actual error log in that case). but bascially MiMa is reported the "ReversedMissingMethodProblem" along with the message "super.tryOfferN is present only in the current version" if I have only

override def tryOfferN(list: List[A])(implicit F: Monad[F]): F[List[A]] = super.tryOfferN(list)

@neomaclin
Copy link
Contributor Author

looks like Mima hates either way....(override with super call triggered the ReversedMissingMethodProblem)

hmm. can you share the full MiMa error message? I don't understand why it's not happy.

or, I could break the build again to generate the error.

@neomaclin
Copy link
Contributor Author

looks like Mima hates either way....(override with super call triggered the ReversedMissingMethodProblem)

hmm. can you share the full MiMa error message? I don't understand why it's not happy.

here they are

[error] cats-effect-std: Failed binary compatibility check against org.typelevel:cats-effect-std_2.13:3.3.11 (e:info.apiURL=https://typelevel.org/cats-effect/api/3.x/, e:info.versionScheme=early-semver)! Found 2 potential problems (filtered 18)
[error]  * abstract synthetic method cats$effect$std$PQueueSink$$super$tryOfferN(scala.collection.immutable.List,cats.Monad)java.lang.Object in interface cats.effect.std.PQueueSink is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("cats.effect.std.PQueueSink.cats$effect$std$PQueueSink$$super$tryOfferN")
[error]  * abstract synthetic method cats$effect$std$PQueueSource$$super$tryTakeN(scala.Option,cats.Monad)java.lang.Object in interface cats.effect.std.PQueueSource is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("cats.effect.std.PQueueSource.cats$effect$std$PQueueSource$$super$tryTakeN")

@neomaclin
Copy link
Contributor Author

fb356b7 with the implementaion on the overrided method was ok with MiMa (and other build checks). let me know if I should go back to that for succcess build of the PR. oh. and since I don't have the write access, I wouldn't be able to merge anything even if the build is/was ok.

@armanbilge
Copy link
Member

We should avoid duplicating the implementation. Since the super trick is not possible, we can extract the implementation to a method on the companion object, and then both traits can delegate to that.

@neomaclin
Copy link
Contributor Author

some how the DispatcherSpec comes back

[error]     x raise an error on leaked runner
[error]      Expected: java.lang.IllegalStateException. Got nothing (DispatcherSpec.scala:269)

@samspills
Copy link
Contributor

some how the DispatcherSpec comes back

[error]     x raise an error on leaked runner
[error]      Expected: java.lang.IllegalStateException. Got nothing (DispatcherSpec.scala:269)

@neomaclin that's unrelated to your PR, don't worry :) It's a known flake: #3776

@neomaclin
Copy link
Contributor Author

if this is good to go in, please go ahead, ( I can't do it myself..)

@djspiewak
Copy link
Member

Thank you for taking this on!

@djspiewak djspiewak merged commit 0f34a2d into typelevel:series/3.x Jan 15, 2024
32 of 36 checks passed
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.

5 participants