lister/tasks: Standardize return statements
ClosedPublic
Actions

Authored by ardumont on Dec 2 2019, 4:01 PM.

Details

Reviewers

olasd

Group Reviewers

Reviewers

Commits

rDLS4a9608f31c24: lister/tasks: Standardize return statements

Summary

The following commit adapts the return statements from both lister and their
associated tasks. This standardizes on what other modules (e.g. both dvcs and
package loaders) do.

It's apparently unnecessary as the listener seems to do what's necessary
already [1] (but it misses tests).

One could argue that having workers (lister, loader, cooker)'s code aligned
would be better in that regard (Note that we could also go the other way
around)

Well, as i started this, might as well propose it...

[1] https://forge.softwareheritage.org/source/swh-scheduler/browse/master/swh/scheduler/celery_backend/listener.py$131-139

Test Plan

tox

Diff Detail

Repository

rDLS Listers

Branch

standardize-return-statement

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 9472
Build 13920: tox-on-jenkins	Jenkins
Build 13919: arc lint + arc unit

Event Timeline

ardumont created this revision.Dec 2 2019, 4:01 PM

Build is green
See https://jenkins.softwareheritage.org/job/DLS/job/tox/506/ for more details.

Harbormaster completed remote builds in B9472: Diff 8398.Dec 2 2019, 4:05 PM

Not a requirement/blocker at all—but while you're at this it would be nice to also add type annotations to capture the fact that these method return something else than None.
That way if the return statements get dropped by mistake in the future, mypy will complain.

Looks good to me.

As for @zack's comment, I think the most useful type annotation would be on the shared_task decorator, rather than annotating individual functions. This way we can type check:

the arguments of functions that end up being tasks, e.g. to force the existence of a given named argument such as url for loading tasks.
the return value of the task (which would be a dict, probably

We could even do specialized decorators depending on the expected shape of the arguments between listing and loading.

I don't know if there's much value adding type annotations to the task functions themselves, as they'll not really be enforced?

This revision is now accepted and ready to land.Dec 3 2019, 11:40 AM

Plug to master branch

ardumont retitled this revision from lister/tasks: Standardize return statements? to lister/tasks: Standardize return statements.Dec 4 2019, 3:16 PM

Build is green
See https://jenkins.softwareheritage.org/job/DLS/job/tox/507/ for more details.

Harbormaster completed remote builds in B9509: Diff 8430.Dec 4 2019, 3:19 PM

Closed by commit rDLS4a9608f31c24: lister/tasks: Standardize return statements (authored by ardumont). · Explain WhyDec 4 2019, 3:21 PM

This revision was automatically updated to reflect the committed changes.

ardumont added a commit: rDLS4a9608f31c24: lister/tasks: Standardize return statements.