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
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 9580
Build 14105: tox-on-jenkinsJenkins
Build 14104: arc lint + arc unit

Event Timeline

  • 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

s/Mapping/Dict/

swh/loader/mercurial/tasks.py
11

Why the (*, notation here ?

11–12

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

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

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

swh/loader/mercurial/tasks.py
11

To force the use of kwargs in the tasks.

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

swh/loader/mercurial/tasks.py
11

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

11–12

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

swh/loader/mercurial/tasks.py
11

Yes, i think so.

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

11–12

Indeed.

I possibly missed them.

Adapt according to review

anlambert added inline comments.
swh/loader/mercurial/tests/test_tasks.py
12–20

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'})
33–40

same here

This revision is now accepted and ready to land.Dec 6 2019, 3:11 PM
swh/loader/mercurial/tests/test_tasks.py
12–20

yup

  • Adapt according to review
  • Amend commit message
  • Plug to master branch