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

Wrap docstrings to 79 chars and check with flake8 #384

Merged
merged 2 commits into from
Nov 28, 2019
Merged

Conversation

leouieda
Copy link
Member

Jupyter, IPython, and other IDEs don't usually render the rst in the
docstrings and just show them literally. The problem is that many of
these break lines at 79 characters. So docstrings with more characters
look terrible in these settings and sometimes almost unreadable. Wrap
all docstrings to 79 characters instead of Black's 88. Set
max-doc-length=79 to make flake8 check if any docstring exceeds it.
The setting checks comments as well and we found no way of disabling
that. So we'll format comments to 79 characters for consistency as well.
Minor modifications to the first line of some docstrings was required to
make them fit into a single line. Also made minor modifications on some
doctests to wrap to 79 characters.

Reminders

  • Run make format and make check to make sure the code follows the style guide.
  • Add tests for new features or tests that would have caught the bug that you're fixing.
  • Add new public functions/methods/classes to doc/api/index.rst.
  • Write detailed docstrings for all functions/methods.
  • If adding new functionality, add an example to docstrings or tutorials.

Jupyter, IPython, and other IDEs don't usually render the rst in the
docstrings and just show them literally. The problem is that many of
these break lines at 79 characters. So docstrings with more characters
look terrible in these settings and sometimes almost unreadable. Wrap
all docstrings to 79 characters instead of Black's 88. Set
`max-doc-length=79` to make flake8 check if any docstring exceeds it.
The setting checks comments as well and we found no way of disabling
that. So we'll format comments to 79 characters for consistency as well.
Minor modifications to the first line of some docstrings was required to
make them fit into a single line. Also made minor modifications on some
doctests to wrap to 79 characters.
@leouieda
Copy link
Member Author

cc @GenericMappingTools/python-contributors please take note of this when writing your docstrings. flake8 will complain so you can't merge things in by mistake but it's good to know why flake8 is complaining :-)

Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Just one minor suggestion (update a url link), otherwise looks good to merge.

CONTRIBUTING.md Outdated
#### Docstrings

**All docstrings** should follow the
[numpy style guide](https://github.com/numpy/numpy/blob/master/doc/HOWTO_DOCUMENT.rst.txt).
Copy link
Member

Choose a reason for hiding this comment

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

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 catching that!

@leouieda leouieda merged commit 343c7ad into master Nov 28, 2019
@leouieda leouieda deleted the docstring-length branch November 28, 2019 09:33
@leouieda
Copy link
Member Author

Merged. Please note that this will impact all current PRs since they are likely to have conflicts with master now.

weiji14 referenced this pull request in weiji14/pygmt Dec 6, 2019
Wrap grdview docstrings to 79 characters as per #384 and make alias formatting nicer after #383.
@weiji14 weiji14 mentioned this pull request Oct 1, 2020
5 tasks
@weiji14 weiji14 mentioned this pull request Nov 6, 2020
@weiji14 weiji14 mentioned this pull request May 10, 2022
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants