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

Can yield (not return) a list of tasks. #1713

Merged
merged 1 commit into from
Jun 8, 2016
Merged

Conversation

bmaggard
Copy link
Contributor

@bmaggard bmaggard commented Jun 7, 2016

Returning tasks from run(), even in a list, does not add dependencies. yield does.

Description

Clarifying the documentation to specify the proper mechanism by which to add dynamic dependencies.

Motivation and Context

The documentation is incorrect.

Have you tested this? If so, how?

I tested with the following cases. TestYieldTask performs as desired. TestRunTask does not.

class TestYieldTask(DoDynDepTestTask):

    def run(_s):
        yield [ DynDepBTestTask(), DynDepCTestTask(), ]
        super().run()
class TestRunTask(DoDynDepTestTask):

    def run(_s):
        super().run()
        return [ DynDepBTestTask(), DynDepCTestTask(), ]

Returning tasks from run(), even in a list, does not add dependencies.  yield does.
@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @steenzout, @erikbern and @jcrobak to be potential reviewers

@Tarrasch Tarrasch merged commit a4fce14 into spotify:master Jun 8, 2016
@Tarrasch
Copy link
Contributor

Tarrasch commented Jun 8, 2016

Thanks. Usually for these small docs fixes, you can get away with a more lightly motivated PR (as the maintainers know already if you must yield or return). :)

While at it, I guess it should say "yield any iterable", not only "yield a list"

@bmaggard bmaggard deleted the patch-1 branch June 9, 2016 01:40
This was referenced Jun 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants