Page MenuHomeSoftware Heritage

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

Authored by ardumont on Tue, Nov 5, 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
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.Tue, Nov 5, 12:02 PM
douardda added inline comments.Tue, Nov 5, 12:29 PM
swh/lister/cli.py
204 ↗(On Diff #7646)

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...

ardumont added inline comments.Tue, Nov 5, 5:02 PM
swh/lister/cli.py
204 ↗(On Diff #7646)

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 ;)

ardumont updated this revision to Diff 7661.Tue, Nov 5, 5:40 PM

lister.debian: Move run method parameters to constructor

ardumont updated this revision to Diff 7662.Tue, Nov 5, 5:45 PM
  • 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
220

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–31

Shows the distribution is actually passed to the constructor now.

ardumont added inline comments.Tue, Nov 5, 5:48 PM
swh/lister/debian/lister.py
59

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–31

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.Tue, Nov 5, 5:56 PM
ardumont edited the summary of this revision. (Show Details)
ardumont edited the summary of this revision. (Show Details)Tue, Nov 5, 6:27 PM
douardda accepted this revision.Wed, Nov 6, 11:26 AM
This revision is now accepted and ready to land.Wed, Nov 6, 11:26 AM