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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ardumont created this revision.Dec 2 2019, 4:01 PM
zack added a subscriber: zack.Dec 2 2019, 6:06 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.

olasd accepted this revision.Dec 3 2019, 11:40 AM

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 updated this revision to Diff 8430.Dec 4 2019, 3:16 PM

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
This revision was automatically updated to reflect the committed changes.