Page MenuHomeSoftware Heritage

loader.cli: Reference new cli
ClosedPublic

Authored by ardumont on Dec 4 2019, 7:45 PM.

Details

Summary

This installs a swh loader subcommand.
This allows loading of url for a given loader.

This also aligns the npm loader's constructor to be consistent with other
loaders (having only one url as constructor parameter).

Related to T2134

Diff Detail

Repository
rDLDBASE Generic VCS/Package 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 4 2019, 7:45 PM
ardumont edited the summary of this revision. (Show Details)Dec 4 2019, 7:46 PM
ardumont edited the summary of this revision. (Show Details)Dec 4 2019, 7:48 PM
ardumont edited the summary of this revision. (Show Details)Dec 5 2019, 8:26 AM
ardumont retitled this revision from loader.cli: Reference new loader cli to loader.cli: Reference new cli.Dec 5 2019, 9:37 AM
ardumont updated this revision to Diff 8441.EditedDec 5 2019, 9:38 AM
  • loader.cli: Add tests around cli

(that still cannot pass green here though, need to land the loaders' first D2395)

ardumont updated this revision to Diff 8450.Dec 5 2019, 12:35 PM
  • loader.cli: Reference new loader cli
  • package.npm: Align loader instantiation
  • loader.cli: Add tests around cli
ardumont updated this revision to Diff 8452.Dec 5 2019, 2:27 PM
  • requirements-test: Add missing test dependency
ardumont updated this revision to Diff 8456.Dec 5 2019, 4:09 PM

Rebase on latest master

ardumont edited the summary of this revision. (Show Details)Dec 6 2019, 11:47 AM
anlambert accepted this revision.Dec 6 2019, 1:21 PM
anlambert added a subscriber: anlambert.

Looks good to me !

swh/loader/cli.py
62

Load an origin from its url with loader <name>

swh/loader/package/npm/loader.py
49 ↗(On Diff #8456)

nitpick: it's better to use concrete type (here Dict) when typing return value

swh/loader/tests/test_cli.py
82 ↗(On Diff #8456)

Triggering

85 ↗(On Diff #8456)

this should be removed I think

93 ↗(On Diff #8456)

same here

This revision is now accepted and ready to land.Dec 6 2019, 1:21 PM
ardumont updated this revision to Diff 8475.Dec 6 2019, 1:45 PM

Adapt according to review

ardumont updated this revision to Diff 8476.EditedDec 6 2019, 1:53 PM

Adapt according to review (i missed one)

ardumont updated this revision to Diff 8477.Dec 6 2019, 2:04 PM
  • Fix test on cli (forgot to amend the test after changing the cli's help message)
  • Use pytest-mock's mocker fixture in tests needing to patch behavior
ardumont added inline comments.Dec 6 2019, 2:05 PM
requirements-test.txt
2 ↗(On Diff #8477)

It's packaged in debian stable so ok: python3-pytest-mock ;)

ardumont updated this revision to Diff 8478.Dec 6 2019, 2:06 PM

Plug to master branch

anlambert added inline comments.Dec 6 2019, 2:08 PM
swh/loader/tests/test_cli.py
80 ↗(On Diff #8478)

Still this one to replace with mocker fixture

ardumont updated this revision to Diff 8479.Dec 6 2019, 2:10 PM

Migrate last test with mocker fixture (missed)