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

api: Add consume_first for convenience #502

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vinzenz
Copy link
Member

@vinzenz vinzenz commented May 3, 2019

A lot of actors are expecting only one type and are using next to
retrieve the value. This patch introduces a convenience function that is
simplifying the usage for actors that have a need for this.

Additionally it covers some gotchas that are mostly not handled, mainly
if there is no message available then an empty tuple is returned that is
not iterable by next.

Usage example:

from leapp.libraries.stdlib import api
from leapp.models import SomeModel

def some_function_previously():
    value = next(api.consume(SomeModel), None)
    value_other = next(api.consume(SomeModel), SomeModel())
    value_different = next(api.consume(SomeModel), 'yadda')

def some_function_now():
    value = api.consume_first(SomeModel)
    value_other api.consume_first_default(SomeModel)
    value_different = api.consume_first(SomeModel, 'yadda')

Signed-off-by: Vinzenz Feenstra vfeenstr@redhat.com

@vinzenz vinzenz force-pushed the consume_first branch 5 times, most recently from 665a860 to cbcdd5a Compare May 3, 2019 10:17
A lot of actors are expecting only one type and are using next to
retrieve the value. This patch introduces a convenience function that is
simplifying the usage for actors that have a need for this.

Additionally it covers some gotchas that are mostly not handled, mainly
if there is no message available then an empty tuple is returned that is
not iterable by next.

Usage example:
--------------

```
from leapp.libraries.stdlib import api
from leapp.models import SomeModel

def some_function_previously():
    value = next(api.consume(SomeModel), None)
    value_other = next(api.consume(SomeModel), SomeModel())
    value_different = next(api.consume(SomeModel), 'yadda')

def some_function_now():
    value = api.consume_first(SomeModel)
    value_other api.consume_first_default(SomeModel)
    value_different = api.consume_first(SomeModel, 'yadda')
```

Signed-off-by: Vinzenz Feenstra <vfeenstr@redhat.com>
@vinzenz vinzenz requested a review from a team May 3, 2019 11:14
@shaded-enmity
Copy link
Member

Wondering if it makes sense to check if there's actually more messages and maybe emit a log message if there is, i.e. Discarded 3 MyModel messages in somerepo.Someactor?

@vinzenz
Copy link
Member Author

vinzenz commented May 3, 2019

Wondering if it makes sense to check if there's actually more messages and maybe emit a log message if there is, i.e. Discarded 3 MyModel messages in somerepo.Someactor?

That'd be something like expect_one or so, no?

@shaded-enmity
Copy link
Member

Yeah, for me consume_first entails expect_one as well - what I mean, thinking from a long term maintenance perspective, if I say consume_first and at some point in time another actor is added which produces that message, suddenly there are two messages for me to consume, however I have no knowledge of the contents / semantics of the second message, and even if I did, I probably don't have control over which message will be produced first.
It is also more explicit in terms of expectancies - if I see actor use expect_one I can be pretty sure that adding a second message will not play along nicely with the logic the actor implements.

@pirat89
Copy link
Member

pirat89 commented May 3, 2019

+1 for expect_one or something like that. But it creates impression in my mind, that one msg is always expected. And there doesn't have to be any. So maybe another naming would be better yet.

Edit: or True/False paramter can be added.

@leapp-bot
Copy link
Collaborator

Thank you for contributing to the Leapp project!

Please note that every PR needs to comply with the Leapp Guidelines, pass tests and linter checks before it can be merged.

If you want to re-run tests or request review, you can use following commands as a comment:

  • leapp-ci build to run unit tests and copr build
  • e2e tests to run unit tests, copr build and end-to-end tests (OAMG members only)
  • review please to notify leapp developers of review request

@fernflower
Copy link
Member

That's a nice to have thing, maybe we could revive it?
FWIW, consume_one naming makes more sense to me :)

@pirat89
Copy link
Member

pirat89 commented Apr 1, 2022

FWIW, consume_one naming makes more sense to me :)

@fernflower I think we could do ~ cabal mtg after we finish everything around the current release to discuss extension of the provided API. or we could create something somewhere where we could discuss it. I mean, not just this, but in general from our experience, whether we could identify more functions we that could make sense to put into the library.

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

Successfully merging this pull request may close these issues.

None yet

5 participants