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

Correctly detect publicity of modules inside directories #494

Merged
merged 25 commits into from Aug 22, 2020
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
3b730ac
Fix Module.is_public() when the module is not a the root
tibdex May 4, 2020
be1aafe
Add PR reference
tibdex May 4, 2020
fcca361
Use pathlib
tibdex Jul 6, 2020
716332e
Merge branch 'master' into fix-module_is_public
tibdex Jul 6, 2020
eb201f3
Use pathlib instead of os for test_module_publicity
gbroques Jul 19, 2020
0bac2fc
Update release notes for #493
gbroques Jul 19, 2020
60e8ba0
Resolve merge conflicts in release_notes.rst
gbroques Jul 19, 2020
10e1e7a
Use forward slash '/' operator instead of .joinpath()
gbroques Jul 20, 2020
c911e9f
Fix pull-request number in release notes
gbroques Jul 20, 2020
123025c
Fix publicity of module in private package
gbroques Jul 20, 2020
8c07f7f
Update test_module_publicity docstring
gbroques Jul 21, 2020
84fa77f
Add test for directory starting with double underscore
gbroques Jul 27, 2020
44d661c
Make packages containing double-underscore public
gbroques Aug 1, 2020
a7f7b6c
Add test to assert __init__ module is public
gbroques Aug 1, 2020
ef7f5a1
Make modules in a __private_package private
gbroques Aug 1, 2020
53a246e
Fix lint errors from lines being too long
gbroques Aug 1, 2020
6d62651
Update publicity.rst with more information
gbroques Aug 1, 2020
cec5793
Parameterize module publicity tests and include .py file extension in…
gbroques Aug 1, 2020
4b57b42
Make module publicity determination respect $PYTHONPATH
gbroques Aug 6, 2020
f807365
Fix line-length issue
gbroques Aug 6, 2020
4bb734d
Resolve merge conflicts in release_notes.rst
gbroques Aug 6, 2020
cbf1c60
Reword comment
gbroques Aug 7, 2020
dde693f
Add tests with the same path over different sys.path cases
gbroques Aug 7, 2020
2f6ab9e
Add note about checking sys.path for determining publicity
gbroques Aug 10, 2020
0e5648c
Apply suggestions from code review
samj1912 Aug 22, 2020
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
3 changes: 3 additions & 0 deletions .gitignore
Expand Up @@ -54,3 +54,6 @@ docs/snippets/error_code_table.rst

# PyCharm files
.idea

# VS Code
.vscode/
4 changes: 2 additions & 2 deletions docs/release_notes.rst
Expand Up @@ -20,8 +20,8 @@ Bug Fixes
The bug caused some argument names to go unreported in D417 (#448).
* Fixed an issue where skipping errors on module level docstring via #noqa
failed when there where more prior comments (#446).
* Support backslash-continued descriptions in docstrings
(#472).
* Support backslash-continued descriptions in docstrings (#472).
* Correctly detect publicity of modules inside directories (#470, #494).


5.0.2 - January 8th, 2020
Expand Down
10 changes: 9 additions & 1 deletion docs/snippets/publicity.rst
Expand Up @@ -10,7 +10,9 @@ Publicity for all constructs is determined as follows: a construct is
considered *public* if -

1. Its immediate parent is public *and*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also update the docs to include our new behavior?

Copy link
Contributor Author

@gbroques gbroques Aug 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which new behavior are you referring to?

Do you have suggestions for wording it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah - I meant the behavior around syspath. That may catch some people off guard.

Copy link
Contributor Author

@gbroques gbroques Aug 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can mention it as a "Note" somewhere in the "How publicity is determined" section.

Note, a construct's parent is recursively checked upward until we reach a directory in sys.path to avoid considering the complete filepath.

Thoughts?

EDIT: I suppose this is dependent upon your other comment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep this comment to what you suggested for now.

Copy link
Contributor Author

@gbroques gbroques Aug 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in commit 2f6ab9e.

2. Its name does not contain a single leading underscore.
2. Its name does *not* start with a single or double underscore.

a. Note, names that start and end with a double underscore are *public* (e.g. ``__init__.py``).

A construct's immediate parent is the construct that contains it. For example,
a method's parent is a class object. A class' parent is usually a module, but
Expand All @@ -25,6 +27,12 @@ a class called ``_Foo`` is considered private. A method ``bar`` in ``_Foo`` is
also considered private since its parent is a private class, even though its
name does not begin with a single underscore.

Note, a construct's parent is recursively checked upward until we reach a directory
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

construct -> module?

samj1912 marked this conversation as resolved.
Show resolved Hide resolved
in ``sys.path`` to avoid considering the complete filepath of a module.
For example, consider the module ``/_foo/bar/baz.py``.
If ``PYTHONPATH`` is set to ``/``, then ``baz.py`` is *private*.
If ``PYTHONPATH`` is set to ``/_foo/``, then ``baz.py`` is *public*.

Modules are parsed to look if ``__all__`` is defined. If so, only those top
level constructs are considered public. The parser looks for ``__all__``
defined as a literal list or tuple. As the parser doesn't execute the module,
Expand Down
33 changes: 32 additions & 1 deletion src/pydocstyle/parser.py
@@ -1,10 +1,12 @@
"""Python code parser."""

import sys
import textwrap
import tokenize as tk
from itertools import chain, dropwhile
from re import compile as re
from io import StringIO
from pathlib import Path

from .utils import log

Expand Down Expand Up @@ -117,7 +119,36 @@ def is_public(self):

This helps determine if it requires a docstring.
"""
return not self.name.startswith('_') or self.name.startswith('__')
module_name = Path(self.name).stem
return (
not self._is_inside_private_package() and
self._is_public_name(module_name)
)

def _is_inside_private_package(self):
"""Return True if the module is inside a private package."""
path = Path(self.name).parent # Ignore the actual module's name
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a dot at the end of the comment please (my bad I guess).

samj1912 marked this conversation as resolved.
Show resolved Hide resolved
syspath = [Path(p) for p in sys.path] # Convert to pathlib.Path.

# Bail if we are at the root directory or in `PYTHONPATH`.
while path != path.parent and path not in syspath:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should iterate all the way to the root. But only till the current directory? Thoughts?

Copy link
Member

@samj1912 samj1912 Aug 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

edit - The current working directory will be in the sys path in certain conditions https://docs.python.org/3.8/library/sys.html#sys.path

Copy link
Contributor Author

@gbroques gbroques Aug 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some tests with the pydocstyle CLI locally, and the directory that you execute pydocstyle from is not included in sys.path.

I don't think we should iterate all the way to the root. But only till the current directory?

Maybe your right? Please advise.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am just wondering about this use case because if we iterate all they way till root, there might be folders with an underscore in their name. On the other hand this means that pydocstyle will have different outputs on the same input based on the CWD which I am not sure about. So - let's keep it to sys.path for now simply because it makes the checker consistent.

In the future if we decide to change this behavior we can always put in a PR - @Nurdok what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for keeping as is and documenting it. A user can always get the current-directory-behavior by calling PYTHONPATH=$PYTHONPATH:$CWD pydocstyle ....

if self._is_private_name(path.name):
return True
path = path.parent

return False

def _is_public_name(self, module_name):
"""Determine whether a "module name" (i.e. module or package name) is public."""
return (
not module_name.startswith('_') or (
module_name.startswith('__') and module_name.endswith('__')
)
)

def _is_private_name(self, module_name):
"""Determine whether a "module name" (i.e. module or package name) is private."""
return not self._is_public_name(module_name)

def __str__(self):
return 'at module level'
Expand Down
71 changes: 64 additions & 7 deletions src/tests/parser_test.py
Expand Up @@ -4,6 +4,7 @@
import sys
import pytest
import textwrap
from pathlib import Path

from pydocstyle.parser import Parser, ParseError

Expand Down Expand Up @@ -562,19 +563,75 @@ def test_matrix_multiplication_with_decorators(code):
assert inner_function.decorators[0].name == 'a'


def test_module_publicity():
"""Test that a module that has a single leading underscore is private."""
@pytest.mark.parametrize("public_path", (
Path(""),
Path("module.py"),
Path("package") / "module.py",
Path("package") / "__init__.py",
Path("") / "package" / "module.py",
Path("") / "__dunder__" / "package" / "module.py"
))
def test_module_publicity_with_public_path(public_path):
"""Test module publicity with public path.

Module names such as my_module.py are considered public.

Special "dunder" modules,
with leading and trailing double-underscores (e.g. __init__.py) are public.

The same rules for publicity apply to both packages and modules.
"""
parser = Parser()
code = CodeSnippet("")

module = parser.parse(code, "filepath")
module = parser.parse(code, str(public_path))
assert module.is_public

module = parser.parse(code, "_filepath")

@pytest.mark.parametrize("private_path", (
# single underscore
Path("_private_module.py"),
Path("_private_package") / "module.py",
Path("_private_package") / "package" / "module.py",
Path("") / "_private_package" / "package" / "module.py",

# double underscore
Path("__private_module.py"),
Path("__private_package") / "module.py",
Path("__private_package") / "package" / "module.py",
Path("") / "__private_package" / "package" / "module.py"
))
def test_module_publicity_with_private_paths(private_path):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also add some tests with the same path over different sys.path cases.

Copy link
Contributor Author

@gbroques gbroques Aug 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate on how to do this please?


EDIT: It looks like pytest includes a monkeypatch fixture with syspath_prepend() method to modify sys.path.

Is something like this desirable (updated in dde693f)?

@pytest.mark.parametrize("syspath,is_public", (
    ("/", False),
    ("_foo/", True),
))
def test_module_publicity_with_different_sys_path(syspath, is_public, monkeypatch):
    """Test module publicity for same path and different sys.path."""
    parser = Parser()
    code = CodeSnippet("")

    monkeypatch.syspath_prepend(syspath)
    
    path = Path("_foo") / "bar" / "baz.py"
    module = parser.parse(code, str(path))
    assert module.is_public == is_public

"""Test module publicity with private path.

Module names starting with single or double-underscore are private.
For example, _my_private_module.py and __my_private_module.py.

Any module within a private package is considered private.

The same rules for publicity apply to both packages and modules.
"""
parser = Parser()
code = CodeSnippet("")
module = parser.parse(code, str(private_path))
assert not module.is_public

module = parser.parse(code, "__filepath")
assert module.is_public

@pytest.mark.parametrize("syspath,is_public", (
("/", False),
("_foo/", True),
))
def test_module_publicity_with_different_sys_path(syspath,
is_public,
monkeypatch):
"""Test module publicity for same path and different sys.path."""
parser = Parser()
code = CodeSnippet("")

monkeypatch.syspath_prepend(syspath)

path = Path("_foo") / "bar" / "baz.py"
module = parser.parse(code, str(path))
assert module.is_public == is_public


def test_complex_module():
Expand Down