Page MenuHomeSoftware Heritage

lister/tasks: Standardize return statements
ClosedPublic

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

Details

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-jenkinsJenkins
Build 13919: arc lint + arc unit

Event Timeline

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
ardumont retitled this revision from lister/tasks: Standardize return statements? to lister/tasks: Standardize return statements.Dec 4 2019, 3:16 PM