Opened 8 years ago

Closed 8 years ago

#5027 enhancement closed fixed (fixed)

FilePath Documentation Patch

Reported by: Corey Richardson Owned by:
Priority: normal Milestone:
Component: core Keywords: filepath documentation
Cc: Thijs Triemstra Branch: branches/filepath-docstrings-5027
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun, KB1PKL

Description (last modified by Thijs Triemstra)

Filled out some docstrings in twisted.python.filepath.FilePath, addressed ticket #4736. Didn't modify anything (besides adding short blurb about unicode in FilePath.__doc__), just added.

Attachments (2)

filepath-api-docs.patch (4.2 KB) - added by Corey Richardson 8 years ago.
Diff of my changes.
filepath-api-docs.2.patch (4.9 KB) - added by Corey Richardson 8 years ago.
Fixed issues with the new documentation.

Download all attachments as: .zip

Change History (8)

Changed 8 years ago by Corey Richardson

Attachment: filepath-api-docs.patch added

Diff of my changes.

comment:1 Changed 8 years ago by Corey Richardson

Keywords: review added

comment:2 Changed 8 years ago by Thijs Triemstra

Author: KB1PKL
Cc: Thijs Triemstra added
Description: modified (diff)
Keywords: documentation added; review removed
Owner: set to Corey Richardson

Thanks for your patch. Couple of things:

  1. C{FilePath} should be L{FilePath}
  2. docstrings should be on 3 lines instead of 1

comment:3 Changed 8 years ago by Corey Richardson

Keywords: review added
Owner: Corey Richardson deleted

Fixed the issues (C{ should be L{, docstrings on three lines).

Changed 8 years ago by Corey Richardson

Attachment: filepath-api-docs.2.patch added

Fixed issues with the new documentation.

comment:4 Changed 8 years ago by Jean-Paul Calderone

Author: KB1PKLexarkun, KB1PKL
Branch: branches/filepath-docstrings-5027

(In [31646]) Branching to 'filepath-docstrings-5027'

comment:5 Changed 8 years ago by Jean-Paul Calderone

Keywords: review removed

Thanks for the patch KB1PKL.

  1. Don't ever use tabs for indentation! Only use spaces.
  2. I don't think Initialize the L{FilePath} tells the reader much that they didn't already know. More useful would be something like Convert a path string to an absolute path if necessary and initialize the L{FilePath} with the result.
  3. Since there still are no unit tests demonstrating that unicode paths work, the docstring shouldn't say that they do.
  4. sibbling is misspelled in the sibling docstring; also it would be better to define the behavior of the method without using the name of the method. Instead of sibling, say that the resulting path has the same directory but a different base name.
  5. There's some trailing whitespace in the patch, we try to avoid that.
  6. The child docstring for path isn't quite correct. The path is the base name for the new FilePath to create.
  7. String isn't a type :) str is though.
  8. The isabs docstring is a little suboptimal, because isabs is a trick. There is no such thing as a relative FilePath, they are all absolute.
  9. For raise epytext markup, the exception type goes before the :. So, raise OSError: ... not raise: OSError ....
  10. FilePath.touch doesn't actually fail silently. If the open('a').close() fails, then either it's because the path couldn't be created, in which case the utime call will fail loudly, or because of some other problem that doesn't matter because utime will still succeed, or some problem which will also cause utime to fail, which will also be loud.
  11. For the basename docstring, seperator is actually spelled separator
  12. It would be excellent for the dirname docstring to be parallel to the basename docstring, since the functionality is somewhat parallel.

These are all fairly minor points. I'll correct them and apply the patch. Thanks again!

comment:6 Changed 8 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [31650]) Merge filepath-docstrings-5027

Author: KB1PKL, exarkun Reviewer: exarkun Fixes: #5027

Add docstrings for many of FilePath's undocumented methods.

Note: See TracTickets for help on using tickets.