Skip to content
This repository has been archived by the owner on Nov 3, 2023. It is now read-only.

Use module filepath basename sans ext in is_public #326

Closed
wants to merge 2 commits into from
Closed

Use module filepath basename sans ext in is_public #326

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ed13725
Use module filepath basename sans ext in is_public
sqwishy Jul 27, 2018
9cfe711
Line in release_notes referencing PR #326
sqwishy Jul 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/release_notes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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).

Copy link
Member

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.


2.1.1 - October 9th, 2017
Expand Down
4 changes: 3 additions & 1 deletion src/pydocstyle/parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
"""Python code parser."""

import logging
import os.path
import six
import textwrap
import tokenize as tk
Expand Down Expand Up @@ -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))
Copy link
Member

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.

Copy link
Author

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.

Copy link
Member

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?

return not module_name.startswith('_') or module_name.startswith('__')

def __str__(self):
return 'at module level'
Expand Down
6 changes: 6 additions & 0 deletions src/tests/parser_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

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).


module = parser.parse(code, "directory/_filepath")
assert not module.is_public


def test_complex_module():
"""Test that a complex module is parsed correctly."""
Expand Down