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

Polish and withCompletionHandler #17

Merged
merged 20 commits into from Aug 14, 2019

Conversation

sideeffffect
Copy link
Member

No description provided.

neko-kai
neko-kai previously approved these changes Aug 12, 2019
Copy link
Member

@neko-kai neko-kai left a comment

Choose a reason for hiding this comment

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

👍 One question, but I think it's up to you what to do here (changing the signature would break the API...)

@sideeffffect
Copy link
Member Author

yeah, this is a breaking PR, there are many things being changed around


import scala.concurrent.ExecutionException

object java {
Copy link
Member

Choose a reason for hiding this comment

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

@sideeffffect Eh, I'd avoid that. What if someone does import zio.interop._ ? Then all subsequent import java.... & java.Smth references will fail. It's the same reason we use zio.interop.catz, not zio.interop.cats – to avoid clashes with a root package.

Copy link
Member Author

Choose a reason for hiding this comment

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

javaz?

Copy link
Member

Choose a reason for hiding this comment

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

OK :D Although I think the old name isn't so bad either

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 wanted to get rid of the concurrent part, because in the future I would like this library to be interop for all things JRE 8, not just for java.util.concurrent

Copy link
Member

Choose a reason for hiding this comment

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

OK 👍

@sideeffffect
Copy link
Member Author

@Kaishh anything left to do before final approval and merge?

Copy link
Member

@neko-kai neko-kai left a comment

Choose a reason for hiding this comment

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

@sideeffffect
Hmm, don't know – it looks good to me, AFAIK you wrote the initial version in the first place? You may merge in case a ZIO invite reached you already, otherwise I may

@sideeffffect
Copy link
Member Author

I don't seem to be able to merge, can you please do it @Kaishh

@pshirshov pshirshov merged commit d8edc2e into zio:master Aug 14, 2019
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

3 participants