Page MenuHomeSoftware Heritage

Fix `cli` module by passing the expected argument
ClosedPublic

Authored by Alphare on Mon, Feb 22, 3:04 PM.

Details

Summary

There is currently no end-to-end test to catch this regression.
I'm not certain whether there is something in place to write such tests,
but this is already better to have a working cli.

Test Plan

Should there be a simple end-to-end test, like the .t files we have in
mercurial itself?

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

Build is green

Patch application report for D5125 (id=18327)

Rebasing onto d8f28b70fa...

Current branch diff-target is up to date.
Changes applied before test
commit 72661ea8632b4345dbce7f1f4fcd8a9cf0ca62ed
Author: Raphaël Gomès <rgomes@octobus.net>
Date:   Mon Feb 22 15:01:06 2021 +0100

    Fix `cli` module by passing the expected argument
    
    There is currently no end-to-end test to catch this regression.
    I'm not certain whether there is something in place to write such tests,
    but this is already better to have a working cli.

See https://jenkins.softwareheritage.org/job/DLDHG/job/tests-on-diff/153/ for more details.

ardumont added a subscriber: ardumont.

Nice catch.

Should there be a simple end-to-end test, like the .t files we have in
mercurial itself?

Well, it's not currently covered by test so your change is fine.
Also see [2]

It's more a toy to use locally, see the storage here for example is an in-memory one (no persistence).

Come to think of it now, by the way, the visit-date input should also be wrong, if provided it should be parsed the same way the tasks module do now...

[2] And by the way, to actually run the loader through the cli now, one should use:

swh loader run mercurial ...

or

swh loader run mercurial_from_disk

All in all, we should probably deprecate this module... and mention what i just told you...

This revision is now accepted and ready to land.Mon, Feb 22, 3:11 PM

I have no issue with deprecating this module if there is a more canonical way of doing the same thing, the mercurial-specific README will have to be adjusted, I simply tried the first example and got a traceback. :)

I have no issue with deprecating this module if there is a more canonical way of doing the same thing,

Yes, it's the same cli way for loaders now.

swh loader run <loader-type> ...

The list of supported loaders is seen with swh loader list iirc

the mercurial-specific README will have to be adjusted,

Agreed.

I simply tried the first example and got a traceback. :)

And thanks for trying to fix it ;)

This revision was landed with ongoing or failed builds.Tue, Feb 23, 5:09 PM
This revision was automatically updated to reflect the committed changes.

Build is green

Patch application report for D5125 (id=18359)

Rebasing onto ef3a2ba79e...

Current branch diff-target is up to date.
Changes applied before test
commit 9f1243f6529f32ba8049d42c2f97964b8eb4bd30
Author: Raphaël Gomès <rgomes@octobus.net>
Date:   Mon Feb 22 15:01:06 2021 +0100

    Fix `cli` module by passing the expected argument
    
    There is currently no end-to-end test to catch this regression.
    I'm not certain whether there is something in place to write such tests,
    but this is already better to have a working cli.

See https://jenkins.softwareheritage.org/job/DLDHG/job/tests-on-diff/156/ for more details.

Should there be a simple end-to-end test, like the .t files we have in
mercurial itself?

Well, it's not currently covered by test so your change is fine.
Also see [2]

It's more a toy to use locally, see the storage here for example is an in-memory one (no persistence).

Come to think of it now, by the way, the visit-date input should also be wrong, if provided it should be parsed the same way the tasks module do now...

[2] And by the way, to actually run the loader through the cli now, one should use:

swh loader run mercurial ...

or

swh loader run mercurial_from_disk

All in all, we should probably deprecate this module... and mention what i just told you...

D5138 deprecates that cli.
D5132 updates the readme to mention the good cli to use.