Page MenuHomeSoftware Heritage
Differential D6310

opam: Move the state initialization into the get_pages method
ClosedPublic
Actions

Authored by ardumont on Sep 20 2021, 5:44 PM.

Details

Reviewers
olasd
Group Reviewers
Reviewers
Commits
rDLSb69b0b7fd674: opam: Move the state initialization into the get_pages method
Summary

We should avoid side-effects in the constructor as much as possible. That avoids
surprising behavior at object instantiation time. The state if needed must be
initialized into the swh.lister.pattern.Lister.get_pages method, as preconized in the
class docstring.

This also fixes the current test that actually bootstrap a real opam local "clone" in
/tmp.

Test Plan

tox (happy)

Diff Detail

Repository
rDLS Listers
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.Sep 20 2021, 5:44 PM
swh-public-ci added a comment.Sep 20 2021, 5:47 PM

Build is green

Patch application report for D6310 (id=22902)

Rebasing onto c803fc2b59...

Current branch diff-target is up to date.
Changes applied before test
commit 05ad61681276c69a63b871e977059871f705a9a7
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Sep 20 17:41:19 2021 +0200

    opam: Separate side-effects call from the constructor
    
    We should avoid side-effects in the constructor as much as possible. That avoids
    surprising behavior at object instantiation time. The state if needed should be now
    built within the `swh.lister.patter.Lister.prepare` method.
    
    This also fixes the current test that actually bootstrap a real opam local "clone".

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

Harbormaster completed remote builds in B23830: Diff 22902.Sep 20 2021, 5:47 PM
ardumont requested review of this revision.Sep 20 2021, 5:47 PM
ardumont updated this revision to Diff 22906.Sep 20 2021, 5:56 PM

Rebase

ardumont added a child revision: D6312: opam: Make the instance optional and derived from the url.Sep 20 2021, 5:57 PM
swh-public-ci added a comment.Sep 20 2021, 5:59 PM

Build is green

Patch application report for D6310 (id=22906)

Rebasing onto c803fc2b59...

Current branch diff-target is up to date.
Changes applied before test
commit 2d0099bf7206347298e38603130445e79a9bf1dd
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Sep 20 17:41:19 2021 +0200

    opam: Separate side-effects call from the constructor
    
    We should avoid side-effects in the constructor as much as possible. That avoids
    surprising behavior at object instantiation time. The state if needed should be now
    built within the `swh.lister.patter.Lister.prepare` method.
    
    This also fixes the current test that actually bootstrap a real opam local "clone".
    
    Related to T3590

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

Harbormaster completed remote builds in B23834: Diff 22906.Sep 20 2021, 5:59 PM
olasd requested changes to this revision.Sep 20 2021, 6:16 PM
olasd added a subscriber: olasd.

If we want to add this, we should also add it to test_pattern.py tests (which check that the scaffolding actually runs as expected). The documentation of the main pattern class also needs to be updated to add a reference to the new method.

The initial intent in the pattern class was to have any such initialization steps run at the beginning of the main loop, which is the get_pages method. Maybe an explicit prepare method makes sense though.

We should also review other loaders for side effects in the __init__ method. I can at least see the gnu lister pull the tree.json.gz there instead of in get_pages.

This revision now requires changes to proceed.Sep 20 2021, 6:16 PM
ardumont mentioned this in D6313: gnu: Respect the pattern docstring about extra state initialization.Sep 21 2021, 10:27 AM
ardumont added a comment.Sep 21 2021, 10:29 AM

If we want to add this, we should also add it to test_pattern.py tests (which check that the scaffolding actually runs as expected). The documentation of the main pattern class also needs to be updated to add a reference to the new method.

Rereading the docstring in question, i'm sold on using the get_pages for now.
(I don't want to dig into the extra audit right now).
I've opened D6313 to align correctly the gnu lister.

I'll readapt this diff the same way, using the get_pages method to do the right thing.

The initial intent in the pattern class was to have any such initialization steps run at the beginning of the main loop, which is the get_pages method. Maybe an explicit prepare method makes sense though.

sold.

We should also review other loaders for side effects in the init method. I can at least see the gnu lister pull the tree.json.gz there instead of in get_pages.

yes, that part i don't want to dig into immediately.

ardumont retitled this revision from opam: Separate side-effects call from the constructor to opam: Move the state initialization into the get_pages method.Sep 21 2021, 10:35 AM
ardumont edited the summary of this revision. (Show Details)
ardumont updated this revision to Diff 22912.Sep 21 2021, 10:36 AM
ardumont edited the summary of this revision. (Show Details)

Rework implementation according to suggestion.

Drop the extra prepare method and move the state initialization into the get_pages method.

swh-public-ci added a comment.Sep 21 2021, 10:39 AM

Build is green

Patch application report for D6310 (id=22912)

Rebasing onto c803fc2b59...

Current branch diff-target is up to date.
Changes applied before test
commit 0af407999ab4932e4dccf51897e0402d748a3f03
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Sep 20 17:41:19 2021 +0200

    opam: Move the state initialization into the get_pages method
    
    We should avoid side-effects in the constructor as much as possible. That avoids
    surprising behavior at object instantiation time. The state if needed must be
    initialized into the `swh.lister.pattern.Lister.get_pages` method, as preconized in the
    class docstring.
    
    This also fixes the current test that actually bootstrap a real opam local "clone" in
    /tmp.
    
    Related to T3590

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

Harbormaster completed remote builds in B23840: Diff 22912.Sep 21 2021, 10:39 AM
ardumont updated this revision to Diff 22916.Sep 21 2021, 10:47 AM

Actually drop the now "spurious" prepare method, we are using get_pages now.

swh-public-ci added a comment.Sep 21 2021, 10:50 AM

Build is green

Patch application report for D6310 (id=22916)

Rebasing onto c803fc2b59...

Current branch diff-target is up to date.
Changes applied before test
commit c160376b17175bd8fa73adddf2e14b8dcfad947b
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Sep 20 17:41:19 2021 +0200

    opam: Move the state initialization into the get_pages method
    
    We should avoid side-effects in the constructor as much as possible. That avoids
    surprising behavior at object instantiation time. The state if needed must be
    initialized into the `swh.lister.pattern.Lister.get_pages` method, as preconized in the
    class docstring.
    
    This also fixes the current test that actually bootstrap a real opam local "clone" in
    /tmp.
    
    Related to T3590

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

Harbormaster completed remote builds in B23844: Diff 22916.Sep 21 2021, 10:50 AM
olasd accepted this revision.Sep 21 2021, 11:03 AM

Thank you!

This revision is now accepted and ready to land.Sep 21 2021, 11:03 AM
douardda added a subscriber: douardda.Sep 21 2021, 11:04 AM
douardda added inline comments.
swh/lister/opam/lister.py
53

is the ws in get_ pages on purpose?

ardumont added inline comments.Sep 21 2021, 11:18 AM
swh/lister/opam/lister.py
53

nope, fixed, thanks.

ardumont updated this revision to Diff 22922.Sep 21 2021, 11:18 AM

Fix typo

swh-public-ci added a comment.Sep 21 2021, 11:21 AM

Build is green

Patch application report for D6310 (id=22922)

Rebasing onto c803fc2b59...

Current branch diff-target is up to date.
Changes applied before test
commit b69b0b7fd6745cd579acf2e5544c3810217bb542
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Sep 20 17:41:19 2021 +0200

    opam: Move the state initialization into the get_pages method
    
    We should avoid side-effects in the constructor as much as possible. That avoids
    surprising behavior at object instantiation time. The state if needed must be
    initialized into the `swh.lister.pattern.Lister.get_pages` method, as preconized in the
    class docstring.
    
    This also fixes the current test that actually bootstrap a real opam local "clone" in
    /tmp.
    
    Related to T3590

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

Harbormaster completed remote builds in B23851: Diff 22922.Sep 21 2021, 11:21 AM
Closed by commit rDLSb69b0b7fd674: opam: Move the state initialization into the get_pages method (authored by ardumont). · Explain WhySep 21 2021, 11:23 AM
This revision was automatically updated to reflect the committed changes.
ardumont added a commit: rDLSb69b0b7fd674: opam: Move the state initialization into the get_pages method.

Revision Contents
Changeset List

PathSize
swh/
lister/
opam/
12 lines
tests/
30 lines

Diff 22926

View Options

swh/lister/opam/lister.py

Loading...
View Options

swh/lister/opam/tests/test_lister.py

Loading...
About · Developer information · Legal