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 Cancellable utility method #34

Merged
merged 6 commits into from
Apr 4, 2022
Merged

Add Cancellable utility method #34

merged 6 commits into from
Apr 4, 2022

Conversation

mdedetrich
Copy link
Contributor

Adds a best effort Cancellable utility method for being able to cancel underlying Future's. Also adds a basic unit spec to test this functionality.

@coveralls
Copy link

coveralls commented Mar 30, 2022 •
edited
Loading

Coverage Status

Coverage decreased (-2.9%) to 86.294% when pulling 7950275 on mdedetrich:add-cancellable into 01e93ee on johanandren:main.

@mdedetrich mdedetrich force-pushed the add-cancellable branch 8 times, most recently from 484a92f to bda0e5e Compare March 30, 2022 10:53
johanandren
johanandren reviewed Apr 1, 2022
View reviewed changes
Copy link
Owner

@johanandren johanandren left a comment

Choose a reason for hiding this comment

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

Neat!

src/main/scala/markatta/futiles/Cancellable.scala Outdated Show resolved Hide resolved
@mdedetrich mdedetrich force-pushed the add-cancellable branch 3 times, most recently from 9d5a04a to a260e63 Compare April 3, 2022 09:59
@mdedetrich
Copy link
Contributor Author

mdedetrich commented Apr 3, 2022 •
edited
Loading

So I just submitted the changes so that we use the future as a delegate and I changed Cancellable to CancellableFuture to better reflect the change. I ended up having to put different implementations for the various Scala versions because the Future interface in Scala 2.11 is different from other Scala versions (transform has a different signature and transformWith doesn't exist)

Not sure if you want the delegate to be private or public? Typically from what I have seen delegates tend to be exposed but I don't have strong thoughts either way.

johanandren
johanandren approved these changes Apr 4, 2022
View reviewed changes
Copy link
Owner

@johanandren johanandren left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

src/main/scala/markatta/futiles/CancellableFuture.scala
*/
@throws[CancellationException]
def cancel(): Unit
}
Copy link
Owner

Choose a reason for hiding this comment

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

Much nicer API 👍

@johanandren
Copy link
Owner

(Since the impl is package private the delegate method won't be publicly accessible)

@johanandren johanandren merged commit 5aa58ab into johanandren:main Apr 4, 2022
@mdedetrich mdedetrich deleted the add-cancellable branch April 4, 2022 08:57
@mdedetrich
Copy link
Contributor Author

(Since the impl is package private the delegate method won't be publicly accessible)

Ah yes forgot about that.

@mdedetrich mdedetrich mentioned this pull request Apr 4, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johanandren johanandren johanandren 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.
3 participants
@mdedetrich @coveralls @johanandren