Skip to content

Rewrite docstrings in Google style, fix #67 #88

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

Merged
merged 24 commits into from
Oct 11, 2017
Merged

Rewrite docstrings in Google style, fix #67 #88

merged 24 commits into from
Oct 11, 2017

Conversation

althonos
Copy link
Member

@althonos althonos commented Oct 6, 2017

Hi Will,

this large PR only addresses documentation and docstrings.

Google style

Every docstring was rewritten in Google style.

pydocstyle

A pydocstyle section was added to setup.cfg, ignoring doc errors that conflict with Google style (this was made by hand, until PyCQA/pydocstyle#275 is fixed). If you want to, it's possible add pydocstyle to .travis.yml if you want to enforce the documentation style.

Cross references

Using intersphinx, the documentation now has a lot of cross-references to the official documentation. All return types now link to their documentation page, either in the stdlib, or in the PyFilesystem2 reference. I also fixed a few cross-referencies, like wrong return values.

Default role

Sphinx uses :py:obj as the default role, which means the documentation can be a lot lighter (:func:'fs.open_fs' becomes simply 'fs.open_fs' (with inverted quotes, but Markdown won't let me show that)). This increases readability when reading the documentation with pydocstyle and help().

Exceptions

The inheritancy of exceptions is now shown in the HTML documentation. Some function / methods were not listing exceptions, so I added them (but still, functions do not provide a comprehensive list).

Module docstrings

Every public module now has a docstring with at least a title. Some of the documentation that was in the rST sources but not in the source code was moved back to the source, so that help() users can see them as well. This was mostly the case for module descriptions. RST style titles (underlined) were moved to the doc source.

fs.filesize

Added explanations and examples to our old friend fs.filesize, as well as examples about who's using which convention (turns out traditional, binary and decimal map to Windows, Linux, and Mac OS X > 10.6 !). fs.filesize was not listed in the reference, so I added it.

Documentation order

For some modules where it made sense, I made the HTML show the functions / classes in source order instead of alphabetic order. This made sense for instance for fs.subfs, where it makes more sense to show SubFS before ClosingSubFS, since the latter is a subclass of the former.

Examples

I added some examples here and there, especially when the behaviour of the function or the method was easier to show that to explain. All the examples are written in doctest format (although, they won't pass as doctests because of missing imports. but we could solve that and add doctests to the unit tests, I did that on a few of my projects !)

About D102

Because pydocstyle is a static checker, it doesn't know about inheritance. This meant that it raised D102: missing method docstring to about every inherited FS method. Two ways to fix that:

  • add # noqa: D102 to every method with a missing docstring. I started doing that, but it crapped the code, and was tedious to do.
  • ignore D102, which I ended up doing. I used sphinx.ext.coverage to check for documentation coverage instead, but since it still reports FS methods as missing docstrings, I dropped it.

So in the end, I don't check for missing function / method docstrings.

NB

  • I realise this will take some time to merge. I'll try to keep the PR up-to-date and rebase against any other commit you may push before that, so neither rush this nor consider this PR a priority.
  • English is not my mother tongue, so I probably messed with the language in locations where I rewrote the documentation.
  • I was not sure about the line length you're trying to enforce, the default rule for my editor is 80 characters, but PEP8 goes for 72 chars with docstrings, so I should probably go back fix a few lines.

@coveralls
Copy link

coveralls commented Oct 6, 2017

Coverage Status

Coverage decreased (-0.8%) to 99.141% when pulling d073216 on althonos:feat-docstring into d22fba6 on PyFilesystem:master.

@coveralls
Copy link

coveralls commented Oct 6, 2017

Coverage Status

Coverage increased (+3.0e-05%) to 99.95% when pulling f791d50 on althonos:feat-docstring into d22fba6 on PyFilesystem:master.

@coveralls
Copy link

coveralls commented Oct 6, 2017

Coverage Status

Coverage increased (+3.0e-05%) to 99.95% when pulling cf1c39c on althonos:feat-docstring into d22fba6 on PyFilesystem:master.

@willmcgugan
Copy link
Member

@althanos This is great. If this is just docs, I don't seem harm in merging sooner rather than later. But I'll go through it shortly.

I do have a couple of questions re docstring formatting (purely questions, not asking for changes).

I've noticed you insert a newline at the end of perviously-single line docstrings.

So this...

"""Base class for FS objects."""

Becomes:

"""Base class for FS objects.
"""

Any particular reason for that? PEP 257 is ok with single line docstrings.

I've also noticed you remove the first newline in the docstring, where I tend to insert a blank line so the indentation is constant. As far as I can tell PEP 257 is ok with that -- "The summary line may be on the same line as the opening quotes or on the next line."

@willmcgugan
Copy link
Member

I was not sure about the line length you're trying to enforce, the default rule for my editor is 80 characters, but PEP8 goes for 72 chars with docstrings, so I should probably go back fix a few lines.

I was going for 79 as a maximum for code, and 72 for docstrings where possible. Although I'm not overly concerned with line length. I have a sublime text plugin that auto wraps text when I press Cmd+W.

@althonos
Copy link
Member Author

The Google guide explicitly supports having the summary on the first line even for multi-line docstrings, so that's what I did.

Putting the closing quotes on another line is not enforced however, it's just a personal habit (I tend to find it more consistent, but that's just me).

@willmcgugan
Copy link
Member

@althonos looks good to me. Will merge as soon as Travis is done.

@coveralls
Copy link

coveralls commented Oct 11, 2017

Coverage Status

Coverage increased (+3.0e-05%) to 99.95% when pulling 9abeaf2 on althonos:feat-docstring into d2da568 on PyFilesystem:master.

@willmcgugan willmcgugan merged commit a23e87f into PyFilesystem:master Oct 11, 2017
@willmcgugan
Copy link
Member

Thanks!

@althonos
Copy link
Member Author

You're welcome !

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.

3 participants