Page MenuHomeSoftware Heritage

lister.debian: Move parameters from `run` method to constructor
ClosedPublic

Authored by ardumont on Nov 5 2019, 12:02 PM.

Details

Summary

This was initially to align the lister's cli with the scheduler's (and it is now).

In the end, still discussing with @douardda, that has also become an alignment on the run method (only the debian one needed changes).

Now no lister pass any parameters to the run command.
And those needed parameters are passed to the constructors instead.

And with those changes, the initial goal, be able to run:

swh lister run --lister debian distribution=Debian

is still possible ;)

Test Plan

tox

Diff Detail

Repository
rDLS Listers
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 8813
Build 12844: tox-on-jenkinsJenkins
Build 12843: arc lint + arc unit

Event Timeline

swh/lister/cli.py
204

this is weird, now you pass the kw to both the lister constructor and the run() method. Is it expected to work?

This reminds me of the discussion we had about the fact we want the run() method to never take arguments so we do not have this kind of argument dispatching between init() and run() to do...

swh/lister/cli.py
204

this is weird, now you pass the kw to both the lister constructor and the run() method. Is it expected to work?

Yes, it is weird but i kept that because that was the original behavior.
I opened the args, kw solely for the lister debian.
It's the only one needing it.

Is it expected to work?

Yes and it will.
The unused keys in the configuration are just that, unused (depending on each lister's implementation).

This reminds me of the discussion we had about the fact we want the run() method to never take arguments so we do not have this kind of argument dispatching between init() and run() to do...

Indeed.

mmm

Like i just said, only the debian lister is taking parameter...
So, i might as well do it now... I think ;)

lister.debian: Move run method parameters to constructor

  • Fix typing (missing an optional)
  • improve docstrings (be explicit)
  • revert to existing code where there is no need to touch (if it ain't broke...)
swh/lister/debian/lister.py
208

That actually fixes a current bug in the logs.
When the distribution is not registered (without the fix), that displays 'Distribution None is not registered'.

swh/lister/debian/tests/test_tasks.py
30 ↗(On Diff #7661)

Shows the distribution is actually passed to the constructor now.

swh/lister/debian/lister.py
45

Now we move the weirdness in the constructor though...

That's to tiptoe around the get_lister which initialize lister (only the override_config is passed there).
So here, i'm reading first on override_config (used in the cli context) and fallback to the constructor parameters if passed...
At last, i fallback on default values if nothing is provided.

swh/lister/debian/tests/test_tasks.py
30 ↗(On Diff #7661)

and no longer to the run method.

ardumont retitled this revision from lister.cli: Pass parameters from cli to lister run method to lister.debian: Move parameters from `run` method to constructor.Nov 5 2019, 5:56 PM
ardumont edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Nov 6 2019, 11:26 AM