Correctly detect publicity of modules inside directories #494

gbroques · 2020-07-19T23:19:17Z

Continuation of PR #470 as @tibdex was taking a little to respond, and their PR relates to issue #493 that I opened.

I branched off @tibdex's work so they get contribution credits for the work they did. Thank you @tibdex!!

@samj1912 @Nurdok Please review. This uses pathlib in the parser_test.py now.

src/tests/parser_test.py

docs/release_notes.rst

gbroques · 2020-07-20T22:48:01Z

@samj1912 What's your expectation of these tests?

    module = parser.parse(code, str(parent_path / "_private_pkg" / "filepath"))
    assert not module.is_public

    module = parser.parse(code, str(parent_path / "_private_pkg" / "some_pkg" / "filepath"))
    assert not module.is_public

My expectation is that these should pass ✔️? (the above tests currently fail ❌)

Thus, a module containing no leading underscore inside a private package should be considered private.

EDIT: Nevermind, according to @Nurdok in this comment and other comments in PR #326, these should be private.

This is addressed in commit 123025c.

gbroques · 2020-07-21T00:38:50Z

Publicity is inherently affected by where you run pydocstyle.

Consider the following example:

publicpackage/
├── __init__.py
└── _privatepackage/
    └── some_private_module.py  # no docstring here

$ pydocstyle publicpackage/ ↵
  # no errors

$ cd publicpackage/_privatepackage ↵
$ pydocstyle some_private_module.py ↵
some_private_module.py:1 at module level:
        D100: Missing docstring in public module

Maybe we can simply document that publicity is affected by where pydocstyle is executed?

This issue was also brought up in #326 by this comment.

@Nurdok @samj1912 thoughts?

tibdex · 2020-07-27T10:54:52Z

I was on vacation, thanks for taking over @gbroques 👍. I'll close my PR.

gbroques · 2020-07-27T13:57:04Z

I was on vacation, thanks for taking over @gbroques 👍. I'll close my PR.

Not a problem! Thank you for your initial work on this.

Hopefully we can get it merged :)

src/pydocstyle/parser.py

Nurdok · 2020-08-06T06:32:14Z

src/pydocstyle/parser.py

+        path = Path(self.name).parent  # Ignore the actual module's name
+        syspath = [Path(p) for p in sys.path]  # Convert to pathlib.Path.
+
+        while path != path.parent and path not in syspath:  # Bail if we are at the root directory or in `PYTHONPATH`.


Sorry for the error in my own code, but I guess this exceeds 80 chars? Just move the comment one line up.

No worries, this should be resolved in f807365.

Nurdok · 2020-08-06T06:33:21Z

src/tests/parser_test.py

+    Path("__private_package") / "package" / "module.py",
+    Path("") / "__private_package" / "package" / "module.py"
+))
+def test_module_publicity_with_private_paths(private_path):


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

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

samj1912 · 2020-08-08T15:00:48Z

docs/snippets/publicity.rst

@@ -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*


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

Which new behavior are you referring to?

Do you have suggestions for wording it?

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

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.

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

Updated in commit 2f6ab9e.

samj1912 · 2020-08-08T15:03:22Z

src/pydocstyle/parser.py

+        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:


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

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

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.

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?

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

samj1912

@gbroques Thank you for being so persistent and patient about this PR. I think this is good to go but I would like @Nurdok to take a look before merging since he had the last significant review on this PR.

gbroques · 2020-08-10T23:31:57Z

@gbroques Thank you for being so persistent and patient about this PR. I think this is good to go but I would like @Nurdok to take a look before merging since he had the last significant review on this PR.

No worries! I really appreciate all the excellent feedback you and @Nurdok have provided, and this is just a small way I can say thank you for the work you two do. :)

Nurdok · 2020-08-22T17:02:57Z

docs/snippets/publicity.rst

@@ -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


construct -> module?

Nurdok · 2020-08-22T17:03:48Z

src/pydocstyle/parser.py

+
+    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


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

docs/snippets/publicity.rst

src/pydocstyle/parser.py

samj1912 · 2020-08-22T17:50:34Z

@gbroques sorry for the long wait. Thanks for this PR!

gbroques · 2020-08-22T20:06:34Z

@samj1912 thank you!

tibdex and others added 6 commits May 4, 2020 14:48

Fix Module.is_public() when the module is not a the root

3b730ac

Add PR reference

be1aafe

Use pathlib

fcca361

Merge branch 'master' into fix-module_is_public

716332e

Use pathlib instead of os for test_module_publicity

eb201f3

Update release notes for PyCQA#493

0bac2fc

This was referenced Jul 19, 2020

Fix Module.is_public() when the module is not a the root #470

Closed

[Bug] D100: Module Containing Single Leading Underscore Considered Public #493

Closed

Resolve merge conflicts in release_notes.rst

60e8ba0

samj1912 suggested changes Jul 20, 2020

View reviewed changes

src/tests/parser_test.py Outdated Show resolved Hide resolved

Use forward slash '/' operator instead of .joinpath()

10e1e7a

samj1912 approved these changes Jul 20, 2020

View reviewed changes

samj1912 previously approved these changes Jul 20, 2020

View reviewed changes

samj1912 reviewed Jul 20, 2020

View reviewed changes

docs/release_notes.rst Outdated Show resolved Hide resolved

Fix pull-request number in release notes

c911e9f

gbroques dismissed samj1912’s stale review via c911e9f July 20, 2020 22:20

Fix publicity of module in private package

123025c

gbroques force-pushed the fix-module_is_public branch from 39e5c4b to 123025c Compare July 20, 2020 23:51

Update test_module_publicity docstring

8c07f7f

gbroques mentioned this pull request Jul 22, 2020

Add pydocstyle to osewb lint gbroques/ose-workbench-platform#44

Open

gbroques requested a review from samj1912 July 23, 2020 00:03

samj1912 reviewed Jul 27, 2020

View reviewed changes

src/pydocstyle/parser.py Outdated Show resolved Hide resolved

Add test for directory starting with double underscore

84fa77f

gbroques requested review from samj1912 and Nurdok July 27, 2020 23:52

Make packages containing double-underscore public

44d661c

gbroques requested a review from samj1912 August 6, 2020 00:13

Nurdok suggested changes Aug 6, 2020

View reviewed changes

gbroques added 2 commits August 6, 2020 18:35

Fix line-length issue

f807365

Resolve merge conflicts in release_notes.rst

4bb734d

gbroques force-pushed the fix-module_is_public branch from fd3ab0d to 4bb734d Compare August 6, 2020 23:44

gbroques requested a review from Nurdok August 6, 2020 23:45

Reword comment

cbf1c60

gbroques force-pushed the fix-module_is_public branch 3 times, most recently from 211088c to 1bda783 Compare August 7, 2020 00:28

Add tests with the same path over different sys.path cases

dde693f

gbroques force-pushed the fix-module_is_public branch from 1bda783 to dde693f Compare August 7, 2020 00:31

samj1912 reviewed Aug 8, 2020

View reviewed changes

samj1912 previously approved these changes Aug 10, 2020

View reviewed changes

gbroques dismissed samj1912’s stale review via b72179d August 10, 2020 23:46

Add note about checking sys.path for determining publicity

2f6ab9e

gbroques force-pushed the fix-module_is_public branch from b72179d to 2f6ab9e Compare August 10, 2020 23:47

samj1912 previously approved these changes Aug 16, 2020

View reviewed changes

Nurdok previously approved these changes Aug 22, 2020

View reviewed changes

samj1912 reviewed Aug 22, 2020

View reviewed changes

docs/snippets/publicity.rst Outdated Show resolved Hide resolved

src/pydocstyle/parser.py Outdated Show resolved Hide resolved

Apply suggestions from code review

0e5648c

samj1912 dismissed stale reviews from Nurdok and themself via 0e5648c August 22, 2020 17:36

samj1912 approved these changes Aug 22, 2020

View reviewed changes

samj1912 merged commit 2aa3aa7 into PyCQA:master Aug 22, 2020

samj1912 mentioned this pull request Aug 23, 2020

Private module definition is inconsistent with private method definition. #414

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly detect publicity of modules inside directories #494

Correctly detect publicity of modules inside directories #494

gbroques commented Jul 19, 2020

gbroques commented Jul 20, 2020 •

edited

gbroques commented Jul 21, 2020 •

edited

tibdex commented Jul 27, 2020

gbroques commented Jul 27, 2020

Nurdok Aug 6, 2020

gbroques Aug 6, 2020

Nurdok Aug 6, 2020

gbroques Aug 6, 2020 •

edited

samj1912 Aug 8, 2020

gbroques Aug 8, 2020 •

edited

samj1912 Aug 8, 2020

gbroques Aug 8, 2020 •

edited

samj1912 Aug 10, 2020

gbroques Aug 10, 2020 •

edited

samj1912 Aug 8, 2020

samj1912 Aug 8, 2020 •

edited

gbroques Aug 8, 2020 •

edited

samj1912 Aug 10, 2020

Nurdok Aug 22, 2020

samj1912 left a comment

gbroques commented Aug 10, 2020

Nurdok Aug 22, 2020

Nurdok Aug 22, 2020

samj1912 commented Aug 22, 2020

gbroques commented Aug 22, 2020

		@@ -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

Correctly detect publicity of modules inside directories #494

Correctly detect publicity of modules inside directories #494

Conversation

gbroques commented Jul 19, 2020

gbroques commented Jul 20, 2020 • edited

gbroques commented Jul 21, 2020 • edited

tibdex commented Jul 27, 2020

gbroques commented Jul 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbroques Aug 6, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbroques Aug 8, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbroques Aug 8, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbroques Aug 10, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samj1912 Aug 8, 2020 • edited

Choose a reason for hiding this comment

gbroques Aug 8, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samj1912 left a comment

Choose a reason for hiding this comment

gbroques commented Aug 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samj1912 commented Aug 22, 2020

gbroques commented Aug 22, 2020

gbroques commented Jul 20, 2020 •

edited

gbroques commented Jul 21, 2020 •

edited

gbroques Aug 6, 2020 •

edited

gbroques Aug 8, 2020 •

edited

gbroques Aug 8, 2020 •

edited

gbroques Aug 10, 2020 •

edited

samj1912 Aug 8, 2020 •

edited

gbroques Aug 8, 2020 •

edited