Page MenuHomeSoftware Heritage

PEP8 refactoring of scanner modules
ClosedPublic

Authored by tenma on Thu, Oct 8, 12:12 PM.

Details

Summary

Also includes:

  • type Iterator instead of Iterable when applicable
  • rename private methods from leading 2 underscores to 1 to match

project coding style

Depends on D4186

Diff Detail

Repository
rDTSCN Code scanner
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

tenma created this revision.Thu, Oct 8, 12:12 PM
vlorentz added a subscriber: vlorentz.EditedThu, Oct 8, 12:13 PM

s/refactoring/reformatting/

Other than that, LGTM

Build is green

Patch application report for D4198 (id=14768)

Could not rebase; Attempt merge onto ccf123a4ce...

Updating ccf123a..f98b821
Fast-forward
 swh/scanner/dashboard/dashboard.py  |  2 +-
 swh/scanner/model.py                | 66 ++++++++++++++++++-------------------
 swh/scanner/scanner.py              |  6 ++--
 swh/scanner/tests/conftest.py       |  6 ++--
 swh/scanner/tests/test_dashboard.py |  2 +-
 swh/scanner/tests/test_model.py     | 32 +++++++++---------
 6 files changed, 56 insertions(+), 58 deletions(-)
Changes applied before test
commit f98b8215d8a2fec0f007c3b10daa1ac291dc3cbd
Author: tenma <tenma+swh@mailbox.org>
Date:   Thu Oct 8 09:27:08 2020 +0200

    PEP8 refactoring of scanner modules
    
    Also includes:
    - type Iterator instead of Iterable when applicable
    - rename private methods from leading 2 underscores to 1 to match
    project coding style

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/68/ for more details.

ardumont accepted this revision.Thu, Oct 8, 12:58 PM
ardumont added a subscriber: ardumont.

lgtm as well, since you are touching some method names, might as well add types to those methods as well.

swh/scanner/model.py
61

add a type str here.

201

might as well add types since you are in a refactoring session ;)

This revision is now accepted and ready to land.Thu, Oct 8, 12:58 PM

Overall ok, but I would have preferred the renaming be in a dedicated revision, separated from type annotation fixes/additions.

swh/scanner/model.py
174

unrelated with this diff, but does this code do what it pretends to do?
Looks to me there a kind of double recursion here (the one from the call to iterage() and the explicit recursion in this snipet).

tenma added inline comments.Thu, Oct 8, 6:17 PM
swh/scanner/model.py
174

Yes double recursion, I noted that for later. Not only this branch is not needed, it is actually harmful to correctness and performance. This function could be dropped altogether because it just does .attributes on top of iterate

douardda added inline comments.Thu, Oct 8, 6:19 PM
swh/scanner/model.py
174

I'm not sure this method is actually useful. It seems to be used twice, in to_dict(). which can be rewritten as (if I'm not mistaken)

return {k: v for node in self.iterate() for k, v in node.attributes.items()}

and in show(), where it can also be reworked as:

print(ndjson.dumps(node.attributes for node in self.iterate())
tenma updated this revision to Diff 14831.Thu, Oct 8, 6:40 PM

Rebase

Build is green

Patch application report for D4198 (id=14831)

Could not rebase; Attempt merge onto ccf123a4ce...

Updating ccf123a..88ded86
Fast-forward
 swh/scanner/dashboard/dashboard.py  |  2 +-
 swh/scanner/model.py                | 66 ++++++++++++++++++-------------------
 swh/scanner/scanner.py              |  6 ++--
 swh/scanner/tests/conftest.py       |  6 ++--
 swh/scanner/tests/test_dashboard.py |  2 +-
 swh/scanner/tests/test_model.py     | 32 +++++++++---------
 6 files changed, 56 insertions(+), 58 deletions(-)
Changes applied before test
commit 88ded8631be989fa5237c5f4ad510babf0b13f8f
Author: tenma <tenma+swh@mailbox.org>
Date:   Thu Oct 8 09:27:08 2020 +0200

    PEP8 renaming of scanner modules
    
    Also rename private methods from leading 2 underscores to 1 to match
    project coding style

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/70/ for more details.

This revision was automatically updated to reflect the committed changes.