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
Details
Details
- Reviewers
ardumont - Group Reviewers
Reviewers - Commits
- rDTSCN88ded8631be9: PEP8 renaming of scanner modules
Diff Detail
Diff Detail
- Repository
- rDTSCN Code scanner
- Branch
- model-refactor
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 16069 Build 24716: Phabricator diff pipeline on jenkins Jenkins console · Jenkins Build 24715: arc lint + arc unit
Event Timeline
Comment Actions
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.
Comment Actions
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? | |
| 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 | |
| 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()) | |
Comment Actions
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.