Opened 16 years ago

Closed 10 years ago

Last modified 10 years ago

#2176 enhancement closed fixed (fixed)

Need IFilePath

Reported by: Wilfredo Sánchez Vega Owned by: Corbin Simpson
Priority: highest Milestone:
Component: core Keywords:
Cc: Wilfredo Sánchez Vega, David Reid, Glyph Branch: branches/ifilepath-2176-3
branch-diff, diff-cov, branch-cov, buildbot
Author: simpson

Description

There needs to be an IFilePath interface, so that alternate implementations can be adapted.

Attachments (4)

2176-0.patch (10.5 KB) - added by Corbin Simpson 10 years ago.
IFilePath, with ZipPath missing getsize()
2176-1.patch (11.3 KB) - added by Corbin Simpson 10 years ago.
IFilePath, with ZipPath fully implementing the interface and all tests passing
2176-2.patch (10.9 KB) - added by Corbin Simpson 10 years ago.
Killed restat in interface (and zippath), removed misleading comment about exceptions
2176-3.patch (11.0 KB) - added by Corbin Simpson 10 years ago.
More #2176 with changed() instead of restat()

Download all attachments as: .zip

Change History (54)

comment:1 Changed 16 years ago by Wilfredo Sánchez Vega

Cc: David Reid added
Status: newassigned

Need this for #2151.

comment:2 Changed 16 years ago by Wilfredo Sánchez Vega

Keywords: review added

branch: ifilepath-2176

comment:3 Changed 16 years ago by Wilfredo Sánchez Vega

Owner: Wilfredo Sánchez Vega deleted
Status: assignednew

comment:4 Changed 16 years ago by Wilfredo Sánchez Vega

Same branch also fixes #2151.

comment:5 Changed 16 years ago by Wilfredo Sánchez Vega

Feedback from glyph & exarkun: Remove the str->FilePath adaptor; it's lame. That's gone now.

comment:6 Changed 16 years ago by Wilfredo Sánchez Vega

Feedback from Jerub: make sure mktab web2 --path still works...

comment:7 Changed 16 years ago by Glyph

Owner: set to Glyph
Status: newassigned

Reviewin'

comment:8 Changed 16 years ago by Glyph

Keywords: review removed
Owner: changed from Glyph to Wilfredo Sánchez Vega
Status: assignednew

Review:

The fact that changes to twisted.web2.test.util can immediately fix all the integration tests that rely on the behavior of File as a fixture is a good thing and indicates good factoring. However, the fact that no individual FilePath tests had to be changed is very bad, and indicates that there need to be more tests for static.File in general which test it as a unit rather than a haphazard requirement of other behavior. For example, this branch in particular should really have some tests which verify that static.File works with non-FilePath providers of IFilePath, such as a dummy test IFilePath provider, to make sure it does not rely on implementation accidents of FilePath.

There are too many methods on IFilePath. Many of them won't apply to non-filesystem-based FilePath objects. It would be good to have fewer methods on that interface and widen it as necessary rather than starting off with a bunch and then reducing it over time. In particular, splitext, islink, listdir, and dirname probably shouldn't be there. Some replacements might be necessary (for example, for MIME-type detection, via the extension) but shouldn't be added until some app code actually needs them. isabs is a hilarious oversight, even on FilePath - it had better always return true :). basename is sort of borderline and the getxtime methods are poorly named, but those are really my fault, and existing code uses them so there's no particularly compelling reason why they should be changed... The new docstrings are great though, I'd appreciate it if they could be copied to the FilePath implementation.

Summary

Good stuff:

  • new docstrings are excellent
  • been looking forward to this for a while
  • it's cool that so few things had to change

Bad stuff:

  • add a few tests; I suggest the following:
    • instantiate File with a dummy test provider of IFilePath and verify that a few methods do the right thing
    • instantiate File with an actual FilePath that looks the same as that and verify the same stuff
    • if you're going to continue supporting strings (and since James is Mr. Web2 and he thinks that's a good idea, that's probably a good idea) then there should be a test which passes a string
  • remove unnecessary methods from IFilePath to unburden future implementors (e.g. see ZipPath)

comment:9 Changed 16 years ago by Wilfredo Sánchez Vega

Cc: Glyph added
Status: newassigned

Additional thoughts:

islink() does seem silly without adding readlink(), but it might be worth going that way, since otherwise you have no access to symlinks. An implementation would just have to return False if symlinks aren't supported in the FS. I guess we could decide not to care, though.

I think splitext() could instead be replaced by a method that returns the extension only, and not the rest of the path.

isdir() I want to keep; we need to be able to find the children of a file path.

I hesitate to copy the docstrings down because I think pydoctor should notice that the method in "inherited" from an interface and use that docstring, so you don't have to copy docstrings and subsequently edit them twice.

I kind of think that siblingExtensionSearch() and childSearchPreauth() are awkward methods for the interface, in hindsight. childSearchPreauth() takes paths which are strings, and I think the only strings that should be required are for base names (ie. direct children). siblingExtensionSearch() seems pretty application-specific, that logic should live in web(2).static, not in a generic API.

comment:10 Changed 16 years ago by Wilfredo Sánchez Vega

OK, I've removed several methods from IFilePath, and changed web2.dav so that it's using a class that's restricted to the IFilePath interface, so that we have some test coverage.

One remaining problem (causing the tests to fail) is that we need a way to get ahold of extended attributes from a path object, and presently, the code assumes that we can ask for the string path in order to create an xattr object.

We've talked in the past about adding some extended attribute access via FilePath. Do we want to do that? Seems neccessary if we want web2.dav to be implementation-independant.

comment:11 Changed 16 years ago by Wilfredo Sánchez Vega

Owner: changed from Wilfredo Sánchez Vega to Glyph
Status: assignednew

glyph: More feedback desired here.

comment:12 Changed 16 years ago by Glyph

Owner: changed from Glyph to Wilfredo Sánchez Vega
Priority: normalhighest

Extended attribute access is a separate interface. However, it should be supported on the same object, if it's supported.

wsanchez: if you need more feedback, use the 'review' keyword and 'highest' priority, or things will languish like this for months :)

Bumping and reassigning to you because I am kind of curious as to where this ended up.

comment:13 Changed 15 years ago by Wilfredo Sánchez Vega

Priority: highestnormal
Status: newassigned

Sorry, I kind of petered out on it, since it's nice-but-not-critical for me, and IIRC it turned into a bigger thing than I have cycles for near-term. I'll see if I can't get some time to re-evaluate.

comment:14 Changed 15 years ago by therve

Milestone: Web2-Gold-Master

comment:15 Changed 15 years ago by dialtone

Line 235 of twisted.web2.dav.static is quite weird since it makes some tests fail for me. Basically all the tests run fast enough that time.time() - mtime is always less than a second away if instead of 1 I use 0.1 all tests pass.

What's the current status of this ticket? It depends on the status of twisted.web2.dav.

comment:16 Changed 15 years ago by Glyph

Owner: changed from Wilfredo Sánchez Vega to dialtone
Status: assignednew

It just needs someone to deal with the review feedback and clean it up. It's mostly a documentation issue; I don't think that we should be mixing in other issues like xattrs in this ticket; I'd like to initially see something that, e.g. the ZipPath implementation could implement simply and obviously.

I have some other ideas about how to deal with xattrs, but I will reserve them for another ticket.

I haven't heard anything from wsanchez about anything in a really long time, let alone this, so you can consider yourself the winner by default ;).

comment:17 Changed 15 years ago by dialtone

Keywords: review added
Priority: normalhighest

I did a pretty superficial comparison between the 2 interfaces (IFilePath and ZipPath) and here's what I found out:

Common methods or attributes:

    sep = Attribute("The path separator to use in string representations")

    def parent(self):
    def child(name): # slight difference here: ZipPath uses a path attribute
    def exists():
    def isdir():
    def isfile():
    def listdir():
    def basename(self):

Missing methods in ZipPath:

    def getsize():
    def children():
    def touch():
    def remove():
    def makedirs():
    def globChildren(pattern):

Different signatures:

    # in IFilePath
    def open(mode="r"):
    def restat(reraise=True):

    # in ZipPath
    def open(self):
    def restat(self):

Methods in ZipPath and not in IFilePath:

    def sibling(self, path):
    def islink(self):
    def splitext(self):
    def dirname(self):

Methods in common but spelled differently:

    # in ZipPath
    def getAccessTime(self):
    def getModificationTime(self):
    def getStatusChangeTime(self):

    # in IFilePath there should be changed since are deprecated
    def getmtime():
    def getctime():
    def getatime():

What's the strategy then? AbstractFilePath should probably be considered in IFilePath since both ZipPath and FilePath implement it. Then there are a number of other methods that should be implemented or updated in the interface. Any priorityies(always take what ZipPath and FilePath have in common rather than anything else for example)?

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

What state is this ticket in? It has the review keyword but is it still up for review? It's assigned to dialtone, but dialtone commented on it most recently? Should someone else be responding to these comments?

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

Owner: changed from dialtone to Glyph

Okay I read the comments and I guess this is waiting for input from someone with interest in IFilePath. :) For now, I'm taking that to be glyph.

comment:20 Changed 15 years ago by Glyph

Keywords: review removed

Some of the comments there aren't correct (for example, ZipPath does have a children() method) but I am not really sure how to respond to the form of the questions posed here.

I guess I'll just deal with the review issues myself and then put it up for review again later.

comment:21 Changed 15 years ago by dialtone

Yes, there are some methods that are inherited by AbstractFilePath.

The basic question is actually if IFilePath should be as small as AbstractFilePath or a bit bigger.

comment:22 Changed 15 years ago by dialtone

Keywords: review added

I need someone to review the current state of IFilePath interface in twisted.python.filepath in ifilepath-2176-2 branch.

comment:23 Changed 15 years ago by Wilfredo Sánchez Vega

There is a little bit of inconsistency with regard to names. getsize() doesn't capitalize Size , unlike getAccessTime() etc. Personally, I think the get prefix is silly, though popular in Java. fp.size(), fp.accessTime(), etc. are my preference, but Python programmers go both ways. I'd have to ask, then, why not getBasename(), getChild() and getParent(), as well...

The deprecated methods getmtime() etc. should be on FilePath, not AbsractFilePath. AbsractFilePath doesn't exist on trunk and therefore has no legacy to protect, so there's no reason to add a brand-new but already deprecated method onto it.

comment:24 Changed 15 years ago by David Reid

Keywords: review removed
Owner: changed from Glyph to dialtone

It looks like wilfredo reviewed this.

I would elaborate on his first point that the big issue here is about consistency. At the very least we should be internally consistent with the naming of these accessors. Does twisted have any sort policy or even a vague inclination regarding this?

comment:25 in reply to:  23 Changed 14 years ago by Glyph

Replying to wsanchez:

There is a little bit of inconsistency with regard to names. getsize() doesn't capitalize Size , unlike getAccessTime() etc. Personally, I think the get prefix is silly, though popular in Java. fp.size(), fp.accessTime(), etc. are my preference, but Python programmers go both ways. I'd have to ask, then, why not getBasename(), getChild() and getParent(), as well...

By the way, I just have to say - I fall prety to this trap all the time, but I am really starting to hate methods with names beginning with get too (despite the fact that I am still constantly naming stuff "getWhatever" all the time).

comment:26 Changed 13 years ago by rotund

Milestone: Web2-Gold-Master

Removed Milestone as Web2 is being merged back into Web. Bug is still important.

comment:27 Changed 10 years ago by Corbin Simpson

What is left to do on this bug? The Web2 complaints are presumably totally invalid now. Do we just need to decide on whether or not to use "get" in the method names?

comment:28 in reply to:  27 Changed 10 years ago by Glyph

Replying to MostAwesomeDude:

What is left to do on this bug? The Web2 complaints are presumably totally invalid now. Do we just need to decide on whether or not to use "get" in the method names?

Grab the branch - although I can't find it at the moment, that's weird - make sure it's up to snuff with respect to tests, etc, and put it up for review again.

You're correct that the web2 comments are mostly irrelevant at this point.

comment:29 Changed 10 years ago by Corbin Simpson

Branch: ifilepath-2176-2

Setting the branch so that it doesn't get lost again.

comment:30 Changed 10 years ago by Jean-Paul Calderone

Branch: ifilepath-2176-2/branches/ifilepath-2176-2

comment:31 Changed 10 years ago by Corbin Simpson

Owner: changed from dialtone to Corbin Simpson

Formally accepting this; it shouldn't be too much work.

I have wsanchez' and dialtone's work rebased on top of the current SVN trunk.

Current issues:

  • verifyObject() doesn't pass for ZipPath/ZipArchive due to getsize() missing. File size is obtainable for those, so I should be able to fix that. There might be other things missing, but I think that's the only one without any implementation.
  • Do we want to formalize the API with "get" before a handful of names? Currently they are: getsize, getModificationTime, getAccessTime, and getStatusChangeTime.
  • I've never seen "status change time" before. Are we keeping that, too? Do we prefer these over the classic atime/mtime/ctime?

I was only going to add tests for verifying that the objects implement the API; there shouldn't be any other untested code in there. Feel free to tell me to fix things.

Changed 10 years ago by Corbin Simpson

Attachment: 2176-0.patch added

IFilePath, with ZipPath missing getsize()

comment:32 in reply to:  31 Changed 10 years ago by Glyph

Replying to MostAwesomeDude:

Formally accepting this; it shouldn't be too much work.

I have wsanchez' and dialtone's work rebased on top of the current SVN trunk.

Cool.

Current issues:

  • verifyObject() doesn't pass for ZipPath/ZipArchive due to getsize() missing. File size is obtainable for those, so I should be able to fix that. There might be other things missing, but I think that's the only one without any implementation.

If you think the API makes sense, and the APIs would be reasonably straightforward, you don't actually have to make ZipPath implement it in this ticket; you can always file another one for "ZipPath should implement IFilePath"

  • Do we want to formalize the API with "get" before a handful of names? Currently they are: getsize, getModificationTime, getAccessTime, and getStatusChangeTime.

I would rather have a more consistent interface, but I think it would be valuable to have an interface that just formally describes what we've already got; doing a new interface would cause some pain (because those methods would have to be deprecated, it would be harder to support multiple Twisted versions once they're removed) it doesn't really solve the problem (the methods are already there) and the benefit is marginal (the aesthetics are slightly nicer). So, for now, I think, the 'get' stays.

  • I've never seen "status change time" before. Are we keeping that, too? Do we prefer these over the classic atime/mtime/ctime?

The "classic" atime/mtime/ctime are not great; we explicitly moved to the more verbose naming on purpose. Again, we probably shouldn't change them just because the methods are already there, but maybe you will find some of this description of the motivations useful in writing docs.

First, note the description on http://docs.python.org/library/os.html#os.stat

atime and mtime are always access time and file contents modification time respectively. But it's more helpful for the method names to be descriptive, I think, especially given the mess that is ctime.

On Windows, "ctime" is creation time. This is what many novice users assume it must be if they don't read the docs too closely. (After all, what other kind of "c" time is there?) But, in actuality, it's the time of the last change to the file's status (or inode, depending on how literally you want to read things, but "man fstat" tends to put it in terms of "status change" so that's what we went with for the name.

Also on Windows, I believe that mtime is actually what ctime is on UNIX, but at any rate, all this fstat-emulation crap is a total lie on that platform, and the real values are in the FILE_ID_BOTH_DIR_INFO structure, which is to say ChangeTime is UNIX-ish ctime and LastWriteTime is UNIX-ish mtime. So it's hypothetically possible that a future implementation of this could give much more correct answers, given a sufficiently expressive interface that clearly and precisely describes the intent of each method.

An even more fun little detail is that BSD-ish UNIXes also provide an st_birthtime attribute which actually is the file's creation time, so you could implement getCreationTime correctly on those platforms. Linux is out of luck for that attribute though, which is why no such method exists yet, it would have to be an optional extension interface.

(Portability and security in the face of weird platform behavior are an important aspect of FilePath's reason for being, so one day I hope we can implement all this stuff, lock it away behind some nice documentation, and I can purge this information which is weighing down my brain.)

I was only going to add tests for verifying that the objects implement the API; there shouldn't be any other untested code in there. Feel free to tell me to fix things.

Sounds about right, since that's the scope of the ticket.

Thanks for picking this up and I hope some of the commentary was useful.

comment:33 Changed 10 years ago by Wilfredo Sánchez Vega

If crime is a mess, perhaps it needn't be in the interface. Remember that the goal is multiple implementations, and messiness like that gets in the way of good implementations. Perhaps if you need ctime, you need a File and not an IFilePath.

comment:34 Changed 10 years ago by Wilfredo Sánchez Vega

Uh… ctime, not crime, but perhaps apropos.

comment:35 in reply to:  33 Changed 10 years ago by Glyph

Replying to wsanchez:

If crime is a mess, perhaps it needn't be in the interface. Remember that the goal is multiple implementations, and messiness like that gets in the way of good implementations. Perhaps if you need ctime, you need a File and not an IFilePath.

Actually yeah, this is a good point. The method can remain on FilePath before we formalize it.

Also, restat() is a bad method and should have been deprecated or something, maybe there's even a ticket for that. The more sensible version of this, in semantics, implementation, and naming, is .changed().

comment:36 Changed 10 years ago by Corbin Simpson

Keywords: review added

Okay, so. Attaching the current all-tests-pass version of this patch, and asking for review. I'm sure people are going to want docstrings tightened up, and I'd be okay with creating changed() and deprecating restat(), although I don't know how many tickets we want to burn on this project. (I'm okay with doing it all in this ticket, if everybody else is.)

Changed 10 years ago by Corbin Simpson

Attachment: 2176-1.patch added

IFilePath, with ZipPath fully implementing the interface and all tests passing

comment:37 Changed 10 years ago by Thijs Triemstra

Owner: Corbin Simpson deleted

comment:38 Changed 10 years ago by washort

Branch: /branches/ifilepath-2176-2branches/ifilepath-2176-2

comment:39 Changed 10 years ago by washort

Branch: branches/ifilepath-2176-2

comment:40 Changed 10 years ago by washort

Keywords: review removed
Owner: set to Corbin Simpson

Let's go ahead and kill restat while we're at it and put in changed. Formalizing an interface that includes a function with semantics that difficult and inefficient should probably not happen.

Docstrings look good to me. I'd probably create tickets instead of XXX/FIXME comments, if anything.

comment:41 Changed 10 years ago by Corbin Simpson

Keywords: review added

'Round and 'round we go. Got rid of restat. The XXX about exceptions doesn't apply since we really can't (sanely) raise OSError ourselves and have no better way to express the kinds of errors we'll find without wrapping OSError, which is also insane. Thus, we're just going to allow Exception to be raised, and not bother attempting to specialize it further.

New patch attached. All tests pass.

Changed 10 years ago by Corbin Simpson

Attachment: 2176-2.patch added

Killed restat in interface (and zippath), removed misleading comment about exceptions

Changed 10 years ago by Corbin Simpson

Attachment: 2176-3.patch added

More #2176 with changed() instead of restat()

comment:42 Changed 10 years ago by washort

Keywords: review removed

+1

comment:43 Changed 10 years ago by Thijs Triemstra

New methods and classes need an @since marker.

comment:44 Changed 10 years ago by simpson

Author: simpson
Branch: branches/ifilepath-2176-3

(In [33752]) Branching to 'ifilepath-2176-3'

comment:45 Changed 10 years ago by simpson

(In [33753]) #2176: IFilePath, an interface for FilePath and ZipPath.

Refs #2176

comment:46 Changed 10 years ago by simpson

(In [33754]) Add @since tag to IFilePath.

Refs #2176

comment:47 Changed 10 years ago by simpson

(In [33755]) Add topfile for #2176.

Refs #2176

comment:48 Changed 10 years ago by simpson

(In [33759]) Remove newline on topfile.

Refs #2176

comment:49 Changed 10 years ago by simpson

Resolution: fixed
Status: newclosed

(In [33761]) Merge ifilepath-2176-3: IFilePath for file path objects

Author: wsanchez, MostAwesomeDude Reviewer: glyph, washort Fixes: #2176

IFilePath formalizes the semantics and API of FilePath. ZipPath implements this API as well. This forms the groundwork for further file path objects in an object-oriented VFS independent of any actual files, and simplifies life for code expecting FilePath but not ZipPath.

comment:50 Changed 10 years ago by Jean-Paul Calderone

  1. Some simple issues. IFilePath interface docstring has some mistakes:

A file path represents a location for a file-like-object and can be organized into a hierarchy; a file path can can children which are themselves file paths.

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

  1. In zippath.py:

# XXX oh man, is this too much hax?

  1. What's with the whitespace in the new tests in test_paths.py?
  2. ZipPath used to have a restat method. It no longer does.
  3. 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.
  4. ZipPath.getsize is untested
  5. Speaking of naming and getsize, why not getSize? isdir and isfile are suspicious too.
  6. Was descendent omitted intentionally or accidentally?
Note: See TracTickets for help on using tickets.