Page MenuHomeSoftware Heritage

Rework loader instantiation logic according to loader core api
ClosedPublic

Authored by ardumont on Feb 15 2021, 6:00 PM.

Details

Summary

The new loader.core now passes all configuration through the constructor.
So no more default configuration only constructor parameters.

Note that this also updated some docstrings and type along the way.

Related to T1410

Test Plan

tox
(failing until swh.loader.core > 0.17 is released)

Diff Detail

Repository
rDLDSVN Subversion (SVN) loader
Branch
improve-instantiation
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 19224
Build 29814: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 29813: arc lint + arc unit

Event Timeline

Build has FAILED

Patch application report for D5075 (id=18116)

Rebasing onto 71e3b44784...

Current branch diff-target is up to date.
Changes applied before test
commit 727189d56849edd8481db1d6d4b7919c1e935aff
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Sun Feb 7 08:45:16 2021 +0100

    Rework loader instantiation logic according to loader core api
    
    Note that this also updated some docstrings and type along the way.

Link to build: https://jenkins.softwareheritage.org/job/DLDSVN/job/tests-on-diff/111/
See console output for more information: https://jenkins.softwareheritage.org/job/DLDSVN/job/tests-on-diff/111/console

Harbormaster returned this revision to the author for changes because remote builds failed.Feb 15 2021, 6:01 PM
Harbormaster failed remote builds in B19220: Diff 18116!

Build has FAILED

Patch application report for D5075 (id=18119)

Rebasing onto 71e3b44784...

Current branch diff-target is up to date.
Changes applied before test
commit 2d6423cfd454afb07f3f309a9076f95300640936
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Sun Feb 7 08:45:16 2021 +0100

    Rework loader instantiation logic according to loader core api
    
    Note that this also updated some docstrings and type along the way.
    
    Related to T1410

Link to build: https://jenkins.softwareheritage.org/job/DLDSVN/job/tests-on-diff/113/
See console output for more information: https://jenkins.softwareheritage.org/job/DLDSVN/job/tests-on-diff/113/console

Harbormaster returned this revision to the author for changes because remote builds failed.Feb 15 2021, 6:03 PM
Harbormaster failed remote builds in B19223: Diff 18119!

Build has FAILED

Patch application report for D5075 (id=18120)

Rebasing onto 71e3b44784...

Current branch diff-target is up to date.
Changes applied before test
commit 69b218f8e22298d21d39b11893c3045bb6cedec8
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Sun Feb 7 08:45:16 2021 +0100

    Rework loader instantiation logic according to loader core api
    
    Note that this also updated some docstrings and type along the way.
    
    Related to T1410

Link to build: https://jenkins.softwareheritage.org/job/DLDSVN/job/tests-on-diff/114/
See console output for more information: https://jenkins.softwareheritage.org/job/DLDSVN/job/tests-on-diff/114/console

Harbormaster returned this revision to the author for changes because remote builds failed.Feb 15 2021, 6:04 PM
Harbormaster failed remote builds in B19224: Diff 18120!

Build has FAILED

Patch application report for D5075 (id=18182)

Rebasing onto 71e3b44784...

Current branch diff-target is up to date.
Changes applied before test
commit 5ad2709b12881a8b8a65488a82d7dfc5a0cbb48c
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Sun Feb 7 08:45:16 2021 +0100

    Rework loader instantiation logic according to loader core api
    
    Note that this also updated some docstrings and type along the way.
    
    Related to T1410

Link to build: https://jenkins.softwareheritage.org/job/DLDSVN/job/tests-on-diff/116/
See console output for more information: https://jenkins.softwareheritage.org/job/DLDSVN/job/tests-on-diff/116/console

Harbormaster returned this revision to the author for changes because remote builds failed.Feb 17 2021, 1:08 PM
Harbormaster failed remote builds in B19283: Diff 18182!

Build is green

Patch application report for D5075 (id=18182)

Rebasing onto 71e3b44784...

Current branch diff-target is up to date.
Changes applied before test
commit 5ad2709b12881a8b8a65488a82d7dfc5a0cbb48c
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Sun Feb 7 08:45:16 2021 +0100

    Rework loader instantiation logic according to loader core api
    
    Note that this also updated some docstrings and type along the way.
    
    Related to T1410

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

anlambert added a subscriber: anlambert.

Looks good to me !

swh/loader/svn/loader.py
75

would be simpler to use an int with a default value of 0 here

The test if self.check_revision will still behave the same.

93

self.check_revision = check_revision if you remove Optional.

This revision is now accepted and ready to land.Feb 17 2021, 5:45 PM
swh/loader/svn/loader.py
75

I should default to 1000 as before to avoid changing behavior.

Nonetheless, ack on your suggestion.

It's not clear why i used Optional here...
Maybe because i was in bot mode :D

Build is green

Patch application report for D5075 (id=18210)

Rebasing onto 71e3b44784...

Current branch diff-target is up to date.
Changes applied before test
commit 7476a936423ff78f5c81af4262d3e82350deb6f0
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Sun Feb 7 08:45:16 2021 +0100

    Rework loader instantiation logic according to loader core api
    
    Note that this also updated some docstrings and type along the way.
    
    Related to T1410

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

swh/loader/svn/loader.py
75

I think you should set it to 0 as default value as revision checks was not enabled by default previously.

swh/loader/svn/loader.py
75

apparently i misremembered, it's not checking by default ¯\_(ツ)_/¯... (also checked swh-site).

75

yes, agreed

Use correct repo to update the diff ¯\_(ツ)_/¯

Build is green

Patch application report for D5075 (id=18213)

Rebasing onto 71e3b44784...

Current branch diff-target is up to date.
Changes applied before test
commit 2c54129bcf4864537d8d13d6645ef777fc1186ef
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Sun Feb 7 08:45:16 2021 +0100

    Rework loader instantiation logic according to loader core api
    
    Note that this also updated some docstrings and type along the way.
    
    Related to T1410

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