Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lint: add pydocstyle python backend #17596

Conversation

AlexTereshenkov
Copy link
Member

@AlexTereshenkov AlexTereshenkov commented Nov 19, 2022

This is work against #15240

pydocstyle is a static analysis tool for checking compliance with Python docstring conventions. It has ~1K stars on GitHub and comes from the PyCQA organization.

The linter is very similar to other Python linters such as bandit. The code is essentially a copy/paste of the bandit's code with minor tweaks:

  • the tool doesn't produce any reports and only emits found issues into the stdout, so code that deals with the report directory is not used
$ pydocstyle *.py --ignore=D100,D300,D400 1>log.log
WARNING: The /home/alexey.tereshenkov/code/pants/pyproject.toml configuration file was ignored, 
because the `toml` package is not installed.

$ cat log.log     
conftest.py:8 in public function `pytest_runtest_makereport`:
        D103: Missing docstring in public function
conftest.py:24 in public function `pytest_configure`:
        D103: Missing docstring in public function
conftest.py:30 in public function `pytest_addoption`:
        D103: Missing docstring in public function
  • the lock file is generated first with build-support/bin/generate_all_lockfiles.sh --tool=pydocstyle and then BUILD file is updated with a resource target.

Tested running this using the "Pants from source" approach on the example-python repository:

$ ./pants_from_sources lint --pydocstyle-args="--count" ::
...
16:52:13.25 [INFO] Completed: Format with Black - black made no changes.
16:52:13.34 [ERROR] Completed: Lint with Pydocstyle - pydocstyle failed (exit code 1).
Partition: ['CPython==3.9.*']
helloworld/__init__.py:1 at module level:
        D104: Missing docstring in public package
helloworld/greet/__init__.py:1 at module level:
        D104: Missing docstring in public package
<redacted>
        D103: Missing docstring in public function
helloworld/translator/translator_test.py:31 in public function `test_unknown_language`:
        D103: Missing docstring in public function
23

✓ black succeeded.
✓ docformatter succeeded.
✓ flake8 succeeded.
✓ isort succeeded.
✕ pydocstyle failed.

Passing configuration file and seeing the difference in errors picked up via the --count flag:

$ cat .pydocstyle.ini 
[pydocstyle]
ignore = D100,D101,D102,D103,D104

$ ./pants_from_sources lint --pydocstyle-config=".pydocstyle.ini" --pydocstyle-args="--count" ::
...
6

$ ./pants_from_sources lint --pydocstyle-args="--count" ::
...
23

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Thanks for getting started on this!!

Comment on lines +62 to +64
config = FileOption(
default=None,
advanced=True,
help="Path to a Pydocstyle config file (http://www.pydocstyle.org/en/stable/usage.html#configuration-files).",
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we also want to use config autodiscovery:

In order for pydocstyle to use a configuration file automatically, it must be named one of the following options.

setup.cfg
tox.ini
.pydocstyle
.pydocstyle.ini
.pydocstylerc
.pydocstylerc.ini
pyproject.toml

config_discovery = BoolOption(
default=True,
advanced=True,
help=lambda cls: softwrap(
f"""
If true, Pants will include any relevant config files during
runs (`.flake8`, `flake8`, `setup.cfg`, and `tox.ini`).
Use `[{cls.options_scope}].config` instead if your config is in a
non-standard location.
"""
),
)

@property
def config_request(self) -> ConfigFilesRequest:
# See https://flake8.pycqa.org/en/latest/user/configuration.html#configuration-locations
# for how Flake8 discovers config files.
return ConfigFilesRequest(
specified=self.config,
specified_option_name=f"[{self.options_scope}].config",
discovery=self.config_discovery,
check_existence=["flake8", ".flake8"],
check_content={"setup.cfg": b"[flake8]", "tox.ini": b"[flake8]"},
)

I'm unclear if those files must be at the root of the repository, vs can be in directories higher up. If you're willing, it would be helpful to experiment to see what the behavior is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for suggesting this, that's super helpful. Yes, this is what the docs say:

When searching for a configuration file, pydocstyle looks for one of the file specified above in that exact order. ini-like configuration files must have a [pydocstyle] section while toml configuration files must have a [tool.pydocstyle] section. If a configuration file was not found, pydocstyle keeps looking for one up the directory tree until one is found or uses the default configuration.

Copy link
Member Author

Choose a reason for hiding this comment

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

http://www.pydocstyle.org/en/stable/usage.html#configuration-files

I am not sure if we can/want to traverse the directory up as the ./pants command will be run from the root repo? So the config files are likely to be defined in the root.

Copy link
Member

Choose a reason for hiding this comment

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

I think they paragraph is ambiguous if it starts looking in CWD or relative to the file argument(s). If the former, then yeah we can only look in the build root. If the latter we have code for that

Copy link
Contributor

Choose a reason for hiding this comment

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

I think they paragraph is ambiguous

@AlexTereshenkov It would be helpful to experiment with this to figure out what the behavior is

Copy link
Member Author

Choose a reason for hiding this comment

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

I have experimented and can confirm that when running pydocstyle helloworld/main.py with the config file located in the parent directory, it does actually walks up the directory tree (outside the repo root)! I find this behavior very surprising as I can't think why I would want my config file to be picked up from /home/user.name when I run pydocstyle in the /home/user.name/code/repo. I'd very much like to avoid having any outside of the repo config files leak into the repo I am working with.

@thejcannon what do you say if we document that Pants will only search for the config files in the repository root (from where the ./pants command ran) OR by using the config provided in the [pydocstyle].config? I think this is what we do for all other formatters/linters, right?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, feel free to elaborate in the docs. FWIW I also expect users to share your expectation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's a great idea. I've extended the help.

src/python/pants/backend/python/lint/pydocstyle/rules.py Outdated Show resolved Hide resolved
@AlexTereshenkov
Copy link
Member Author

@sureshjoshi @thejcannon could you please help me getting this reviewed? Thank you!

@thejcannon
Copy link
Member

Oh also worth mentioning pydocstyle is also flake8-docstrings (if you use flake8)

@AlexTereshenkov AlexTereshenkov force-pushed the feat/add-pydocstyle-python-backend branch from 5442b4d to fa32965 Compare January 7, 2023 05:11
@AlexTereshenkov
Copy link
Member Author

@thejcannon could you please take a look, I hope I addressed your comments. Many thanks! 🤩

Comment on lines +62 to +64
config = FileOption(
default=None,
advanced=True,
help="Path to a Pydocstyle config file (http://www.pydocstyle.org/en/stable/usage.html#configuration-files).",
)
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, feel free to elaborate in the docs. FWIW I also expect users to share your expectation.

@AlexTereshenkov AlexTereshenkov force-pushed the feat/add-pydocstyle-python-backend branch from 03a3741 to 2d2eaec Compare January 17, 2023 21:06
@AlexTereshenkov AlexTereshenkov merged commit 0fe4584 into pantsbuild:main Jan 17, 2023
@AlexTereshenkov AlexTereshenkov deleted the feat/add-pydocstyle-python-backend branch January 17, 2023 21:47
@AlexTereshenkov AlexTereshenkov added this to the 2.15.x milestone Jan 23, 2023
AlexTereshenkov added a commit to AlexTereshenkov/pants that referenced this pull request Jan 23, 2023
@AlexTereshenkov AlexTereshenkov removed this from the 2.15.x milestone Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants