Page MenuHomeSoftware Heritage

mercurial: Unify loader constructor + register worker (tasks, cli)
ClosedPublic

Authored by ardumont on Dec 5 2019, 3:00 PM.

Diff Detail

Repository
rDLDHG Mercurial loader
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 5 2019, 3:00 PM
ardumont updated this revision to Diff 8454.Dec 5 2019, 3:01 PM
  • mercurial.tasks: Rename tasks according to production
ardumont retitled this revision from mercurial: Unify loader constructor + register worker (task, cli) to mercurial: Unify loader constructor + register worker (tasks, cli).Dec 5 2019, 3:15 PM

Looks good !

swh/loader/mercurial/__init__.py
10 ↗(On Diff #8454)

s/Mapping/Dict/

swh/loader/mercurial/tasks.py
11–12

Just curious, why using @shared_task here as @app.task is used in the other loaders ?

12

Why the (*, notation here ?

swh/loader/mercurial/tests/test_tasks.py
6

Same remark as in D2398 and D2400, you should use pytest-mock

ardumont added inline comments.Dec 6 2019, 1:48 PM
swh/loader/mercurial/tasks.py
11–12

To stop importing the scheduler's app state.
We had issues with that up to the point we stop doing that import.

That's what we do in lister and loader-core's tasks.

I'm just aligning as i go along (if i see it ;)

12

To force the use of kwargs in the tasks.

anlambert added inline comments.Dec 6 2019, 1:52 PM
swh/loader/mercurial/tasks.py
11–12

Ack, you should do the same for the git and svn loader then.

12

Oh I see, should the same behavior be ensured in other loader tasks ?

ardumont added inline comments.Dec 6 2019, 1:55 PM
swh/loader/mercurial/tasks.py
11–12

Indeed.

I possibly missed them.

12

Yes, i think so.

Though there comes the issue of migration (at least for recurring tasks ;)

ardumont updated this revision to Diff 8494.Dec 6 2019, 3:00 PM

Adapt according to review

anlambert accepted this revision.Dec 6 2019, 3:11 PM
anlambert added inline comments.
swh/loader/mercurial/tests/test_tasks.py
13–18

How about using this instead ?

res = swh_app.send_task(
        'swh.loader.mercurial.tasks.LoadMercurial',
         kwargs={'url': 'origin_url',
                 'directory': '/some/repo',
                 'visit_date': 'now'})
30–35

same here

This revision is now accepted and ready to land.Dec 6 2019, 3:11 PM
ardumont added inline comments.Dec 6 2019, 3:42 PM
swh/loader/mercurial/tests/test_tasks.py
13–18

yup

ardumont updated this revision to Diff 8497.Dec 6 2019, 3:49 PM
  • Adapt according to review
  • Amend commit message
  • Plug to master branch