Opened 3 years ago

Closed 3 years ago

#5027 enhancement closed fixed (fixed)

FilePath Documentation Patch

Reported by: KB1PKL Owned by:
Priority: normal Milestone:
Component: core Keywords: filepath documentation
Cc: thijs Branch: branches/filepath-docstrings-5027
(diff, github, buildbot, log)
Author: exarkun, KB1PKL Launchpad Bug:

Description (last modified by thijs)

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 KB1PKL 3 years ago.
Diff of my changes.
filepath-api-docs.2.patch (4.9 KB) - added by KB1PKL 3 years ago.
Fixed issues with the new documentation.

Download all attachments as: .zip

Change History (8)

Changed 3 years ago by KB1PKL

Diff of my changes.

comment:1 Changed 3 years ago by KB1PKL

  • Keywords review added

comment:2 Changed 3 years ago by thijs

  • Author set to KB1PKL
  • Cc thijs added
  • Description modified (diff)
  • Keywords documentation added; review removed
  • Owner set to KB1PKL

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 3 years ago by KB1PKL

  • Keywords review added
  • Owner KB1PKL deleted

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

Changed 3 years ago by KB1PKL

Fixed issues with the new documentation.

comment:4 Changed 3 years ago by exarkun

  • Author changed from KB1PKL to exarkun, KB1PKL
  • Branch set to branches/filepath-docstrings-5027

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

comment:5 Changed 3 years ago by exarkun

  • 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 3 years ago by exarkun

  • Resolution set to fixed
  • Status changed from new to closed

(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.