Opened 6 years ago

Last modified 6 years ago

#5549 defect new

Fix IFilePath fallout

Reported by: Corbin Simpson Owned by: Corbin Simpson
Priority: high Milestone:
Component: core Keywords:
Cc: Branch: branches/ifilepath-fixups-5549
branch-diff, diff-cov, branch-cov, buildbot
Author:

Description (last modified by Jean-Paul Calderone)

A handful of problems were identified with the resolution of #2176 post-merge. Since lots of people are around right now sprinting on Twisted, let's try to get this fixed (and we can do so with this follow-up ticket, rather than do the usual revert, because these issues won't hurt anyone if they're in trunk for a short while).

  1. ZipPath used to have a restat method. It no longer does.
  2. sep seems like it could have been named better; separator? segmentDelimiter? sep is just a copy of the os module naming without really thinking about what's going on.
  3. ZipPath.getsize is untested
  4. Speaking of naming and getsize, why not getSize? isdir and isfile are suspicious too.
  5. Was descendent omitted intentionally or accidentally?

Change History (16)

comment:1 Changed 6 years ago by Corbin Simpson

Author: MostAwesomeDude
Branch: branches/ifilepath-fixups-5549

(In [33802]) Branching to 'ifilepath-fixups-5549'

comment:2 Changed 6 years ago by Corbin Simpson

(In [33803]) t.p.filepath: Fix docstrings.

Refs #5549

comment:3 Changed 6 years ago by Corbin Simpson

(In [33804]) t.p.zippath: Un-XXX.

Refs #5549

comment:4 Changed 6 years ago by Corbin Simpson

Author: MostAwesomeDude
Keywords: review added

From #2176:

  1. ZipPath used to have a restat method. It no longer does.
  2. sep seems like it could have been named better; separator? segmentDelimiter? sep is just a copy of the os module naming without really thinking about what's going on.
  3. ZipPath.getsize is untested
  4. Speaking of naming and getsize, why not getSize? isdir and isfile are suspicious too.
  5. Was descendent omitted intentionally or accidentally?

Should these be addressed? Please, tell me your concerns.

comment:5 Changed 6 years ago by Ying Li

  1. I vote we standardize on camel cased getsize/isdir/isfile names, since the get*Time methods are camel cased, and that is also part of the twisted coding standard. (See this)
  1. The docstring mentions:
    A file path has a name which unique identifies it in the context of its
    parent (if it has one); a file path can not have two children with the same
    name.  This name is referred to as the file path's "base name".
    

This and other variables in any implementing classes should probably be documented in the IFilePath docstring with the @ivar tag. (for instance, see the cred interfaces)

  1. #3132 never got resolved, but there was a lot of discussion at the bottom about possible different arguments to FilePath.open. May be worth looking at/thinking about.

comment:6 Changed 6 years ago by Ying Li

Meh, sorry, #3123.

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

Description: modified (diff)

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

Owner: changed from Corbin Simpson to Jean-Paul Calderone
Status: newassigned

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

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Corbin Simpson
Status: assignednew

Thanks for your quick handling of this ticket.

This paragraph is still wonky:

A file path has a name which uniquely identifies it in the context of its parent, if it has one; a file path cannot have two children with the same name. This unique name is referred to as the file path's "base name".

Move two children stuff out. It breaks the reader's attention with respect to the base name information. "This unique name ..." should immediately follow the first clause (which should instead be the first sentence. Lose the semi-colon).

The other changes in the branch look good, but they don't appear to completely address the points I raised.

comment:10 Changed 6 years ago by Corbin Simpson

(In [33825]) t.p.filepath: Adjust docstring to be more readable.

Make the purpose of base names clear, and move the note about uniqueness relative to parents. This makes the sequence of paragraphs flow much better.

Also remove second hyphen in "file-like-object" for consistency and clarity.

Refs #5549

comment:11 Changed 6 years ago by Corbin Simpson

(In [33826]) t.p.zippath: Readd restat() method.

Unnecessarily clobbered by #2176, restored to its former glory.

Refs #5549

comment:12 Changed 6 years ago by Corbin Simpson

(In [33827]) t.test.test_paths: More ZipPath tests.

Refs #5549

comment:13 Changed 6 years ago by Corbin Simpson

(In [33829]) t.p.filepath, t.p.zippath: Rename and deprecate a few methods.

Per discussion on #2176 and #5549, the methods on IFilePath should all be camel-cased to follow standards. isfile, isdir, and getsize are now isFile, isDir, and getSize, and have the appropriate warnings in the docstrings.

This doesn't regress anything here.

Refs #5549

comment:14 Changed 6 years ago by Corbin Simpson

Keywords: review added
Owner: Corbin Simpson deleted

I don't know what to do about descendant. I really don't. I guess it is fine in AbstractFilePath for now since it's fully expressible in terms of the stuff in the interface.

Everything else has been addressed. Reviews, please.

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

Owner: set to Jean-Paul Calderone
Status: newassigned

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

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Corbin Simpson
Status: assignednew
  1. There's a failing unit test - twisted.test.test_paths.FilePathTestCase.test_copyToMissingDestFileClosing
  2. in the same vein as my other naming comments, isDir? That's what we'd like to be stuck with? :) Can you please review all of the new names, even if I didn't explicitly call them out in a review. In particular, sep, which I did comment on earlier, but still seems to be here.
  3. Regarding the IFilePath class docstring
        A file path cannot have two children with the same name, because base
        names are unique.
    
    They're not unique - eg, FilePath("/foo") and FilePath("/foo/foo") have the same base name.

Please double check that all review comments given already are addressed before submitting for review again. Thanks!

Note: See TracTickets for help on using tickets.