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

Clean up Queue Companion Objects #2280

Closed
adamgfraser opened this issue Nov 19, 2019 · 10 comments · Fixed by #2292
Closed

Clean up Queue Companion Objects #2280

adamgfraser opened this issue Nov 19, 2019 · 10 comments · Fixed by #2292
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed queue ZIO Queue ziohackathon Candidate issues for a ZIO Hackathon

Comments

@adamgfraser
Copy link
Contributor

Right now ZQueue describes the most polymorphic form of a Queue but doesn't have a companion object and all of the methods that would normally be on a companion object are instead on Queue. This is confusing and inconsistent with other ZIO data types.

We should rename the existing Queue object to ZQueue and move it into the existing ZQueue.scala file. Then we should create a new Queue object similar to what we have for UIO or other type aliases that delegates to the implementations in the ZQueue object.

@adamgfraser adamgfraser added enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed queue ZIO Queue ziohackathon Candidate issues for a ZIO Hackathon labels Nov 19, 2019
@ghostdogpr
Copy link
Member

That's because you can only create a Queue, not a ZQueue. A ZQueue is obtained only from operations on an existing Queue.

So, if we move constructors to ZQueue, they would still return a Queue, there is no need to duplicate them.

@adamgfraser
Copy link
Contributor Author

Okay. In discussion in #2242 @jdegoes and @regiskuckaertz seemed to think the current situation was less than ideal and want to change it along these lines.

@ghostdogpr
Copy link
Member

It makes sense to move them for consistency but no need to duplicate them since they will have the exact same types. We can add in the package object val Queue = ZQueue.

It's actually how it was initially but this PR made it the way it is now.

@adamgfraser
Copy link
Contributor Author

Got it. You are definitely the expert here. Could you have constructors that create queues that are more polymorphic, like that write a value to the console every time a value is offered to them or have a fixed delay every time a value is taken? Or is the idea that you would create "normal" queues and then apply combinators to generate the more polymorphic ones?

@ghostdogpr
Copy link
Member

ghostdogpr commented Nov 19, 2019

Or is the idea that you would create "normal" queues and then apply combinators to generate the more polymorphic ones?

Yeah I think it is. We could use an alias for the time being and split it if there's a need later.

@adamgfraser
Copy link
Contributor Author

One thing we might want to think about here is binary compatibility. If we have companion objects for Queue and ZQueue we could always add new methods on them in a binary compatible way. If we just have a val Queue = ZQueue and later decided we did want to have separate objects with different methods would that be a breaking change?

@serragnoli
Copy link
Contributor

If we just have a val Queue = ZQueue and later decided we did want to have separate objects with different methods would that be a breaking change?

Just to add my 2 cents, separating the alias into 2 objects in future wouldn't strike me as a breaking change, but an implementation detail.

@adamgfraser
Copy link
Contributor Author

@serragnoli I totally agree with you from a subjective perspective, but my question is more whether it would be binary compatible or not.

@jdegoes
Copy link
Member

jdegoes commented Nov 19, 2019

The contract we have for ZIO & family:

  • ZIO, the class, has all methods on it
  • ZIO, the companion object, has all methods on it
  • UIO, Task, IO, RIO, URIO are all type aliases
  • The companion objects for UIO, Task, etc., have only the methods producing the specified type (e.g. UIO has all methods that accept/return UIO), with fewer numbers of type parameters where possible; these methods just delegate to the ZIO.* methods

This takes effort to maintain, unlike earlier encodings, but it has good user-experience and is very predictable.

@ghostdogpr
Copy link
Member

Okay, my bad, we actually have similar cases with ZIO.succeed that returns a UIO but still defined in ZIO.scala.

I'll do this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed queue ZIO Queue ziohackathon Candidate issues for a ZIO Hackathon
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants