Use module filepath basename sans ext in is_public #326
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this!
CCing @willfrey to also take a look as he wanted to do something similar.
@@ -22,6 +22,8 @@ Bug Fixes | |||
|
|||
* Fixed a false-positive recognition of section names causing D405 to be | |||
reported (#311, #317). | |||
* Fixed using the module filepath (instead of just the basename) to determine | |||
its privateyness causing D100 to be reported incorrectly sometimes. (#326). | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The release notes should generally describe the changed behavior, not the implementation. This can be e.g., "Fixed an issue where module publicity was incorrectly determined by their top-level package" or something similar.
Also, please also include the issue number along with the PR number.
@@ -121,7 +122,8 @@ class Module(Definition): | |||
|
|||
@property | |||
def is_public(self): | |||
return not self.name.startswith('_') or self.name.startswith('__') | |||
module_name, _ = os.path.splitext(os.path.basename(self.name)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I see this and the tests, I think the fix needs to be a little more complex, see How publicity is determined.
We need to look at all the packages in this module's path to determine the publicity, something like
def is_public(self):
def is_public_name(module_name):
return not module_name.startswith('_') or module_name.startswith('__')
return all(is_public_name(name) for name in self.name.split('.'))
Also, I would appreciate a comment describing the logic here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that except Module.name
is actually the filepath. It's a bit confusing though so I think path
or filepath
should be a new attribute on name
should be what module.__name__
evaluates to in python.
The tricky part is getting the module name. Particularly from absolute paths or relative ones run from within a package. Like if run pydocstyle in mylib/_subpackage/
or give it relative filepath, it won't see the _subpackage
part right now.
Can you confirm whether or not the following should all work and are correct?
file.py
module name is package._subpackage.file
and is private:
~ $ pydocstyle ~/project/package/_subpackage/file.py
~/project $ pydocstyle ~/project/package/_subpackage/file.py
~/project/package/_subpackage $ pydocstyle file.py
__init__.py
module name is package
and is public:
~ $ pydocstyle _project/package/__init__.py
~/_project $ pydocstyle package/__init__.py
I don't actually know how that would be implemented, I don't even want to think about namespace packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are some good points. I'm not sure. Perhaps we need to keep this as you implemented and document that the publicity logic only applies up to the module level.
@shacharoo, do you want to weigh in?
@@ -485,6 +485,12 @@ def test_module_publicity(): | |||
module = parser.parse(code, "__filepath") | |||
assert module.is_public | |||
|
|||
module = parser.parse(code, "_directory/filepath") | |||
assert module.is_public |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my previous comment, this should be private.
Also, please add some more complex tests with sub-packages and specifically the example from the original bug (a private module in a public package).
Any chance of this getting landed? As things stand today, |
Closing in favor of #470. |
See issue #323