Follow pep257 strictly

[Radomir Dopieralski]

It would be nice to follow the pep257 (http://www.python.org/dev/peps/pep-0257/) docstrings formatting more strictly. This involves two things:

* Fix the current docstrings to follow pep257,
* Add a check to the gate jobs to make that stay.

There is a number of tools we could use:

* https://pypi.python.org/pypi/flake8-docstrings/0.1.3
* https://pypi.python.org/pypi/pep257/0.2.4
* https://pypi.python.org/pypi/pylama/2.0.2

We need to choose one of them. Any opinions?

[Radomir Dopieralski (deshipu)]

flake8-docstings looks like a best option, because all it requires is adding it to the test-requirements.txt, but unfortunately right now it fails for me with the error:

Traceback (most recent call last):
  File "/home/sheep/devel/tuskar-ui/.venv/bin/flake8", line 9, in <module>
    load_entry_point('flake8==2.0', 'console_scripts', 'flake8')()
  File "/home/sheep/devel/tuskar-ui/.venv/local/lib/python2.7/site-packages/flake8/main.py", line 29, in main
    report = flake8_style.check_files()
  File "/home/sheep/devel/tuskar-ui/.venv/local/lib/python2.7/site-packages/pep8.py", line 1604, in check_files
    self.input_dir(path)
  File "/home/sheep/devel/tuskar-ui/.venv/local/lib/python2.7/site-packages/pep8.py", line 1640, in input_dir
    runner(os.path.join(root, filename))
  File "/home/sheep/devel/tuskar-ui/.venv/local/lib/python2.7/site-packages/flake8/engine.py", line 69, in input_file
    return fchecker.check_all(expected=expected, line_offset=line_offset)
  File "/home/sheep/devel/tuskar-ui/.venv/local/lib/python2.7/site-packages/pep8.py", line 1348, in check_all
    self.check_ast()
  File "/home/sheep/devel/tuskar-ui/.venv/local/lib/python2.7/site-packages/pep8.py", line 1329, in check_ast
    if not noqa(self.lines[lineno - 1]):
IndexError: list index out of range

[Radomir Dopieralski (deshipu)]

https://bitbucket.org/icordasc/flake8-docstrings/issue/6/listerror-in-pep8-after-installing-flake8

This happens when there are empty files (like __init__.py). I'm not sure this is a blocker, as according to pep257 we should never have an empty file -- it should at least have a docstring in it...

[Radomir Dopieralski (deshipu)]

Just to give the idea of the scale of it:

$ pep257 tuskar_ui/api.py 2>&1 | wc -l
117

[Radomir Dopieralski (deshipu)]

Another problem with pep257 is that it doesn't let us disable checks selectively, so it's all-or-nothing.

[Ben Nemec (bnemec)]

I think this probably requires some discussion on openstack-dev. Enforcing strict pep257 compliance when the rest of OpenStack doesn't is a pretty significant divergence and I'm not sure I'm in favor of it. I find some of the pep257 requirements unnecessary (and that's before I knew about the "no empty files" rule :-) so I might prefer just extending our existing docstring hacking rules if we determine there's a need for more docstring consistency.

My personal opinion on docstrings is that unless the formatting is horribly broken in some way, I'm more interested in the content than the formatting, but I'm just one out of thousands of OpenStack developers so I think a wider discussion would be good. I am in favor of pretty strict code formatting rules, so it's possible I could be convinced the same should apply to docstrings. :-)

[Robert Collins (lifeless)]

I don't think we need to follow PEP257 super strictly.

The main thing is the first line separation, because that affects folded views, api summaries and so on.

It's also super obvious to reviewers once they've learnt:

"""foo bar baz

quux
"""

- good

"""foo bar baz
quux
"""

- bad :)

[Radomir Dopieralski (deshipu)]

@lifeless I would hate to require a certain formatting without checking it automatically. If we have an automatic check, we can ensure that .

It seems to me that the best course of action would be to modify the pep257 checker to allow disabling rules selectively, as the pep8 checker allows. This way we could decide which rules make sense for a particular project.

[Robert Collins (lifeless)]

@Radomir - I can understand that. OTOH we have lots of things we enforce that aren't automated, so I don't think there is a reason to /not/ do something just because it's not yet automated.

[Radomir Dopieralski (deshipu)]

@lifeless:

I see no reason why we shouldn't check it, if it's just a question of adding a simple plugin to our checks. If it's that particular rule that you want, we don't need to use pep257, we can just add a pep8 plugin that checks for it. It's trivial to write once you define the rule.

From your comments at https://review.openstack.org/#/c/54744/1/tuskar_ui/workflows.py I'm guessing (because they are not very clear), that what hurts you is the length of the summary line exceeding the single line. That is very easy to check for, although I have doubts if it's practical to require it while we use Sphinx markup, which can be pretty elaborate.

[Radomir Dopieralski (deshipu)]

I proposed a check for that in hacking, should we add it to our local checks before that gets in?

https://review.openstack.org/#/c/55644/

[Viktor Serhieiev (vsergeyev)]

I found no information about dockstring maximum line length in pep257, put pep8 says, that we should limit it to 72 symbols - see - http://www.python.org/dev/peps/pep-0008/#maximum-line-length

Should we support it?