Opened 10 years ago
Last modified 10 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 )
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).
- ZipPath used to have a restat method. It no longer does.
- 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.
- ZipPath.getsize is untested
- Speaking of naming and getsize, why not getSize? isdir and isfile are suspicious too.
- Was descendent omitted intentionally or accidentally?
Change History (16)
comment:1 Changed 10 years ago by
Author: | → MostAwesomeDude |
---|---|
Branch: | → branches/ifilepath-fixups-5549 |
comment:4 Changed 10 years ago by
Author: | MostAwesomeDude |
---|---|
Keywords: | review added |
From #2176:
- ZipPath used to have a restat method. It no longer does.
- 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.
- ZipPath.getsize is untested
- Speaking of naming and getsize, why not getSize? isdir and isfile are suspicious too.
- Was descendent omitted intentionally or accidentally?
Should these be addressed? Please, tell me your concerns.
comment:5 Changed 10 years ago by
- 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)
- 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)
- #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:7 Changed 10 years ago by
Description: | modified (diff) |
---|
comment:8 Changed 10 years ago by
Owner: | changed from Corbin Simpson to Jean-Paul Calderone |
---|---|
Status: | new → assigned |
comment:9 Changed 10 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Jean-Paul Calderone to Corbin Simpson |
Status: | assigned → new |
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 10 years ago by
(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 10 years ago by
comment:13 Changed 10 years ago by
(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 10 years ago by
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 10 years ago by
Owner: | set to Jean-Paul Calderone |
---|---|
Status: | new → assigned |
comment:16 Changed 10 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Jean-Paul Calderone to Corbin Simpson |
Status: | assigned → new |
- There's a failing unit test -
twisted.test.test_paths.FilePathTestCase.test_copyToMissingDestFileClosing
- 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. - 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!
(In [33802]) Branching to 'ifilepath-fixups-5549'