feat: check for both path/foo.py and path/foo/__init__.py being present#39
feat: check for both path/foo.py and path/foo/__init__.py being present#39GabDug wants to merge 1 commit into
Conversation
jwodder
left a comment
jwodder
left a comment
There was a problem hiding this comment.
You also need to add a section to the README describing the new check, when it fails, why it's a problem, and/or how to fix it.
There was a problem hiding this comment.
Testing with actual wheels is only for special cases, and there generally isn't a need to add more test wheels at this time. Instead, the new check should be tested with a test function in test/test_checking.py that just operates on lists of filepaths; cf. the tests already in that file and the "Adding a New Check" section of CONTRIBUTING.md.
Please remove this wheel from your PR's history before requesting a re-review.
| W008 = "Wheel is empty" | ||
| W009 = "Wheel contains multiple toplevel library entries" | ||
| W010 = "Toplevel library directory contains no Python modules" | ||
| W011 = "Wheel contains module and submodule with the same name" |
There was a problem hiding this comment.
| W011 = "Wheel contains module and submodule with the same name" | |
| W011 = "Wheel contains module and package with the same basename" |
| for name, entry in tree.entries.items(): | ||
| if isinstance(entry, Directory): | ||
| if tree.files.get(name + ".py") is not None: | ||
| conflicts.append(entry.path or name) |
There was a problem hiding this comment.
Instead of returning a single failed check that just contains the conflicting directory paths, I feel you should instead return one FailedCheck for each foo.py + foo/__init__.py pair, and the FailedCheck should contains both of those paths.
Cf. the following paragraph from "Adding a New Check" in CONTRIBUTING.md:
Generally, a check method should return at most one
FailedCheckcontaining all of the failing filepaths; only return multipleFailedChecks if the check applies to groups of files. For example, W002 ("Wheel contains duplicate files") returns a separateFailedCheckfor each group of equal files.
| for name, entry in tree.entries.items(): | ||
| if isinstance(entry, Directory): |
There was a problem hiding this comment.
| for name, entry in tree.entries.items(): | |
| if isinstance(entry, Directory): | |
| for name, entry in tree.subdirectories.items(): |
| for name, entry in tree.entries.items(): | ||
| if isinstance(entry, Directory): | ||
| if tree.files.get(name + ".py") is not None: | ||
| conflicts.append(entry.path or name) |
There was a problem hiding this comment.
| conflicts.append(entry.path or name) | |
| # `entry` exists within a directory and thus is not the root directory, and so it has a path. | |
| assert entry.path is not None | |
| conflicts.append(entry.path) |
| check_dir(entry) | ||
|
|
||
| for tree in (contents.purelib_tree, contents.platlib_tree): | ||
| if isinstance(tree, Directory): |
There was a problem hiding this comment.
This line is redundant; contents.purelib_tree and contents.platlib_tree are always Directorys.
2 participants
Hey 👋
Quick MR to close #19
As it's my first contribution here, I'm happy to take any feedback, especially around the wording of the rule.
I've included a wheel for E2E testing, but will try to add more tests if needed.
Thanks,
Gabriel