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

Result of a docstring formatter and some manual fixes on top of it #5624

Closed
wants to merge 2 commits into from

Conversation

Pierre-Sassoulas
Copy link
Member

Type of Changes

Type
✨ New feature
🔨 Refactoring

Description

Add a pre-commit hook to format our docstring automatically for pep257 using https://github.com/myint/docformatter

Possible options:

usage: docformatter [-h] [-i | -c] [-r] [--wrap-summaries length]
                    [--wrap-descriptions length] [--blank]
                    [--pre-summary-newline] [--make-summary-multi-line]
                    [--force-wrap] [--range line line] [--version]
                    files [files ...]

Formats docstrings to follow PEP 257.

positional arguments:
  files                 files to format or '-' for standard in

optional arguments:
  -h, --help            show this help message and exit
  -i, --in-place        make changes to files instead of printing diffs
  -c, --check           only check and report incorrectly formatted files
  -r, --recursive       drill down directories recursively
  -e, --exclude         exclude directories and files by names

  --wrap-summaries length
                        wrap long summary lines at this length; set to 0 to
                        disable wrapping (default: 79)
  --wrap-descriptions length
                        wrap descriptions at this length; set to 0 to disable
                        wrapping (default: 72)
  --blank               add blank line after description
  --pre-summary-newline
                        add a newline before the summary of a multi-line
                        docstring
  --make-summary-multi-line
                        add a newline before and after the summary of a one-
                        line docstring
  --force-wrap          force descriptions to be wrapped even if it may result
                        in a mess
  --range line line     apply docformatter to docstrings between these lines;
                        line numbers are indexed at 1
  --version             show program's version number and exit

Suggested by @DanielNoord #5620 (comment)

@Pierre-Sassoulas Pierre-Sassoulas added Documentation 📗 Maintenance Discussion or action around maintaining pylint or the dev workflow labels Dec 31, 2021
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.13.0 milestone Dec 31, 2021
@DanielNoord
Copy link
Collaborator

DanielNoord commented Dec 31, 2021

@Pierre-Sassoulas Not sure if this is the right tool for the job.

I took a quick look through the repository and some open issues can also be problematic for us.
For one there is:
PyCQA/docformatter#9 with associated PR PyCQA/docformatter#49.
It has been open since 2015 and I feel like it is one of the conventions we would like to enforce.

Another is:
PyCQA/docformatter#75

I'd like to find a good tool to do this, but this one might not be it sadly 😢

@coveralls
Copy link

coveralls commented Dec 31, 2021

Pull Request Test Coverage Report for Build 1898844568

  • 7 of 7 (100.0%) changed or added relevant lines in 7 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage remained the same at 93.994%

Files with Coverage Reduction New Missed Lines %
pylint/lint/run.py 1 72.79%
Totals Coverage Status
Change from base Build 1882850152: 0.0%
Covered Lines: 14931
Relevant Lines: 15885

💛 - Coveralls

examples/custom_raw.py Outdated Show resolved Hide resolved
examples/custom_raw.py Outdated Show resolved Hide resolved
pylint/checkers/__init__.py Outdated Show resolved Hide resolved
pylint/checkers/__init__.py Outdated Show resolved Hide resolved
pylint/checkers/__init__.py Outdated Show resolved Hide resolved
pylint/checkers/utils.py Outdated Show resolved Hide resolved
pylint/checkers/variables.py Outdated Show resolved Hide resolved
pylint/checkers/variables.py Outdated Show resolved Hide resolved
pylint/checkers/variables.py Outdated Show resolved Hide resolved
pylint/extensions/typing.py Outdated Show resolved Hide resolved
@Pierre-Sassoulas Pierre-Sassoulas removed this from the 2.13.0 milestone Dec 31, 2021
Comment on lines 5 to 6
"""Script used to generate the features file before building the actual
documentation."""
Copy link
Member

Choose a reason for hiding this comment

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

Please let's not do that. It's just not worth it. Not everything needs to be perfect.

@DanielNoord
Copy link
Collaborator

@Pierre-Sassoulas I have started working on a little tool to do some of the things that docformatter does.

I think it should be ready for a 0.1.0 somewhere in the coming weeks. If we want to go ahead with a docstring auto formatter we could use it as we'll have more influence on what it changes.

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the add-docstring-formatter-in-pre-commit branch from 4060436 to 708fbaf Compare January 2, 2022 11:49
@Pierre-Sassoulas Pierre-Sassoulas force-pushed the add-docstring-formatter-in-pre-commit branch 2 times, most recently from 1594ff0 to 18d9c67 Compare January 2, 2022 23:13
@Pierre-Sassoulas Pierre-Sassoulas changed the title Add docstring formatter in pre commit Result of a docstring formatter and some manual fixes on top of it Jan 2, 2022
@DanielNoord
Copy link
Collaborator

@Pierre-Sassoulas The latest fix also seems to have affected functional tests? Was this your intention? Are these 6k changes all manual?

@Pierre-Sassoulas
Copy link
Member Author

Pierre-Sassoulas commented Jan 3, 2022

Are these 6k changes all manual?

It's docformatter + manual and semi manual (sed) after that. I should not have touched the functional tests. I'm going to fix that, but I think doing DanielNoord/pydocstringformatter#3 and DanielNoord/pydocstringformatter#5 before would be cleaner. Are you already working on it or can I work on them ?

@DanielNoord
Copy link
Collaborator

Shall we convert this to draft while I work on pydocstringformatter a bit more? You're more than welcome to work on them as well! I'll make you contributor on the repository so you can assign yourself and we don't do double work!

@DanielNoord
Copy link
Collaborator

We're getting close with pydocstringformatter.

Based on this I think we only need:

  • Make splitting of summary and body more robust and then actually turn it on for pylint
  • Do something with max line length (although this is very controversial and PEP 257 is very conservative with max == 72)
  • Do something with "summaries" that span multiple lines. Should we add a period after them? Should we enforce single-line summaries?

I think all of these are already tracked or are on my to-do list so we don't need to open any new issues.

Let's keep this open as a reminder and progress tracker 😄

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the add-docstring-formatter-in-pre-commit branch from 9a4bfdf to e8aeaf3 Compare February 25, 2022 13:20
@Pierre-Sassoulas
Copy link
Member Author

Closing in favor of #5910

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation 📗 Maintenance Discussion or action around maintaining pylint or the dev workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants