Page MenuHomeSoftware Heritage

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

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

Details

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
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 16015
Build 24638: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 24637: arc lint + arc unit

Event Timeline

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.

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

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.

Better with the new commit ID

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.

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

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.

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)

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>

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

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.