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

Let's muse about the semantics of the new func finish(_ hasFailed: Bool) call of v2.0 of Queuer #12

Closed
zykloman opened this issue Nov 1, 2018 · 3 comments

Comments

@zykloman
Copy link

zykloman commented Nov 1, 2018

I really love simplicity of this framework and the way it is heading. However, I'm not entirely sure I like the semantics of the new func finish(_ hasFailed: Bool) call of version 2.0 of Queuer.

At the call side a successful finish of an operation now looks like this:

operation.finish(false)

Since the parameter name is not exposed, my mind reads "operation not finished". If I'm the only person seeing a potential pitfall here, please close the issue and let's forget about it. Otherwise, I suggest to either exposed the parameter name to make obvious what is happening:

operation.finish(hasFailed: false)

Or, what I think would be a more natural approach, change the semantics to func finish(success: Bool):

operation.finish(success: true)

Or maybe event to func finish(success: Bool = true) :

operation.finish()

I think the last option could have some benefits:

  • The most common task of just finishing an operation is the simplest option: operation.finish()
  • If a user needs a more fine grained behavior, she can add details: operation.finish(success: false)
  • A user new to the framework needs not to know, that a retry-logic event exists
  • This approach is compatible to the previous version of the framework
@FabrizioBrancati
Copy link
Owner

FabrizioBrancati commented Nov 1, 2018

Thanks for that great analysis!
I agree with you, using func finish(success: Bool = true) is way better for all your listed benefits.
I also renamed the hasFailed variable to success to better respect these API.
You can try it on the develop branch, let me know what you think!

@zykloman
Copy link
Author

zykloman commented Nov 1, 2018

Thank you very much for the quick reply and for accepting my suggestion! I gave 25a2c1b a quick spin and the changes are looking very good :)

@FabrizioBrancati
Copy link
Owner

Version 2.0.1 has been released 🎉
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants