Page MenuHomeSoftware Heritage
Differential D4186

scanner.model: Fix Tree.toDict to be side-effect free
ClosedPublic
Actions

Authored by tenma on Oct 7 2020, 5:16 PM.

Details

Reviewers
ardumont
douardda
Commits
rDTSCNbb5ccd812a84: model: fix Tree.toDict to be side-effect free
Summary

Remove reliance on default arg child_nodes which is a dict.

It is unused in client code, breaks tests, and is not needed to build
the dict representation of the tree.

This also refines types on related impacted methods which helps
reasoning about them.

Related to D4089

Diff Detail

Repository
rDTSCN Code scanner
Branch
model-refactor
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 16027
Build 24658: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 24657: arc lint + arc unit

Event Timeline

tenma created this revision.Oct 7 2020, 5:16 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptOct 7 2020, 5:16 PM
swh-public-ci added a comment.Oct 7 2020, 5:18 PM

Build is green

Patch application report for D4186 (id=14742)

Rebasing onto ccf123a4ce...

Current branch diff-target is up to date.
Changes applied before test
commit 27b0c291a410fe1ce529c1b7378d67f68f8d666d
Author: tenma <tenma+swh@mailbox.org>
Date:   Wed Oct 7 16:54:43 2020 +0200

    Fix Tree.toDict updating a default arg dict
    
    The default child_nodes dict does not makes sense to build the dict
    representation of the tree, and is never used in client code.

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

Harbormaster completed remote builds in B16015: Diff 14742.Oct 7 2020, 5:18 PM
ardumont added a subscriber: ardumont.Oct 8 2020, 9:22 AM

Fine with the implementation.
Less so with the commit message/diff title and description.

I think the following would be better:

scanner.model: Remove unused Tree.toDict parameter dict_nodes

    It's unused in client code and it is not needed to build the dict
    representation of the tree.

    This also adds types on related impacted methods.
ardumont requested changes to this revision.Oct 8 2020, 9:22 AM
This revision now requires changes to proceed.Oct 8 2020, 9:22 AM
tenma updated this revision to Diff 14760.Oct 8 2020, 11:09 AM

Improved commit message

swh-public-ci added a comment.Oct 8 2020, 11:11 AM

Build is green

Patch application report for D4186 (id=14760)

Rebasing onto ccf123a4ce...

Current branch diff-target is up to date.
Changes applied before test
commit 27b0c291a410fe1ce529c1b7378d67f68f8d666d
Author: tenma <tenma+swh@mailbox.org>
Date:   Wed Oct 7 16:54:43 2020 +0200

    Fix Tree.toDict updating a default arg dict
    
    The default child_nodes dict does not makes sense to build the dict
    representation of the tree, and is never used in client code.

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

Harbormaster completed remote builds in B16026: Diff 14760.Oct 8 2020, 11:11 AM
tenma updated this revision to Diff 14761.Oct 8 2020, 11:17 AM

Better with the new commit ID

swh-public-ci added a comment.Oct 8 2020, 11:19 AM

Build is green

Patch application report for D4186 (id=14761)

Rebasing onto ccf123a4ce...

Current branch diff-target is up to date.
Changes applied before test
commit 7e6e75481f611cae3e905aa16dace770acad2730
Author: tenma <tenma+swh@mailbox.org>
Date:   Wed Oct 7 16:54:43 2020 +0200

    scanner.model: Fix Tree.toDict to be side-effect free
    
    Removes dependance on default arg child_nodes which is a dict.
    
    It is unused in client code, breaks tests and it is not needed to build
    the dict representation of the tree.
    
    This also refines types on related impacted methods which helps
    reasoning about them.

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

Harbormaster completed remote builds in B16027: Diff 14761.Oct 8 2020, 11:19 AM
ardumont accepted this revision.Oct 8 2020, 11:23 AM

i'd replace "dependance" (which is french) to dependency.
but otherwise, thanks, this is clearer now ;)

This revision is now accepted and ready to land.Oct 8 2020, 11:23 AM
tenma updated this revision to Diff 14764.Oct 8 2020, 11:25 AM

Reworded again

swh-public-ci added a comment.Oct 8 2020, 11:27 AM

Build is green

Patch application report for D4186 (id=14764)

Rebasing onto ccf123a4ce...

Current branch diff-target is up to date.
Changes applied before test
commit 9244e34ca9edd44d2858f996d9ed936697ba6549
Author: tenma <tenma+swh@mailbox.org>
Date:   Wed Oct 7 16:54:43 2020 +0200

    scanner.model: Fix Tree.toDict to be side-effect free
    
    Remove reliance on default arg child_nodes which is a dict.
    
    It is unused in client code, breaks tests, and is not needed to build
    the dict representation of the tree.
    
    This also refines types on related impacted methods which helps
    reasoning about them.

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

Harbormaster completed remote builds in B16030: Diff 14764.Oct 8 2020, 11:27 AM
tenma retitled this revision from Fix Tree.toDict updating a default arg dict to scanner.model: Fix Tree.toDict to be side-effect free.Oct 8 2020, 11:33 AM
tenma edited the summary of this revision. (Show Details)
tenma edited the summary of this revision. (Show Details)
tenma edited reviewers, added: douardda; removed: Reviewers.Oct 8 2020, 11:36 AM
tenma added a child revision: D4198: PEP8 refactoring of scanner modules.Oct 8 2020, 12:12 PM
douardda added a comment.Oct 8 2020, 12:15 PM

looks good (did not even notice toDict() is not even a recursive method! so this dict_nodes really makes no sense at all).

<nitpick> To make things even better, it should be split in 2 revisions:

  • annotation stanzas
  • fix

</nitpick>

douardda accepted this revision.Oct 8 2020, 12:16 PM

The split in 2 revisions is not mandatory, just sayin' for good measure.

tenma updated this revision to Diff 14830.Oct 8 2020, 6:39 PM

Rebase

swh-public-ci added a comment.Oct 8 2020, 6:41 PM

Build is green

Patch application report for D4186 (id=14830)

Rebasing onto ccf123a4ce...

Current branch diff-target is up to date.
Changes applied before test
commit bb5ccd812a845943091a81ba85880ed79574ebea
Author: tenma <tenma+swh@mailbox.org>
Date:   Wed Oct 7 16:54:43 2020 +0200

    model: fix Tree.toDict to be side-effect free
    
    Remove reliance on default arg child_nodes which is a dict.
    
    It is unused in client code, breaks tests, and is not needed to build
    the dict representation of the tree.
    
    This also refines types on related impacted methods which helps
    reasoning about them.

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

Harbormaster completed remote builds in B16068: Diff 14830.Oct 8 2020, 6:41 PM
Closed by commit rDTSCNbb5ccd812a84: model: fix Tree.toDict to be side-effect free (authored by tenma). · Explain WhyOct 8 2020, 6:46 PM
This revision was automatically updated to reflect the committed changes.
tenma added a commit: rDTSCNbb5ccd812a84: model: fix Tree.toDict to be side-effect free.

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
swh/
scanner/
12 lines75%
Loading...

Diff 14761

View Options

swh/scanner/model.py

Loading...
About · Developer information · Legal