Opened 14 years ago
Last modified 10 years ago
#3123 enhancement new
Allow filepath.FilePath to open files with specific flags
Reported by: | Ying Li | Owned by: | Ying Li |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | core | Keywords: | |
Cc: | therve, Jean-Paul Calderone | Branch: |
branches/filepath-open-flags-3123
branch-diff, diff-cov, branch-cov, buildbot |
Author: | therve |
Description
Currently, opening a file in filepath.FilePath does not allow you to pass OS flags like read, write, append, truncate, exclusive, etc. Either change open so that it can take said flags, or create a new function that takes said flags.
Attachments (3)
Change History (24)
comment:1 Changed 14 years ago by
Keywords: | review added |
---|---|
Owner: | Ying Li deleted |
Priority: | normal → highest |
Changed 14 years ago by
Attachment: | filepath.open.flags.patch added |
---|
comment:2 Changed 14 years ago by
Cc: | therve added |
---|---|
Keywords: | review removed |
Owner: | set to Ying Li |
Priority: | highest → high |
- open must return a file handle (you probably want to use os.fdopen). It probably needs a test to check this behavior
- the test should be named
test_openWithFlags
- it would be totally awesome if the assert in open could be removed, a meaningful expression raised instead, and a test added. That's not mandatory for this ticket though.
Thanks!
comment:3 Changed 14 years ago by
Keywords: | review added |
---|---|
Owner: | Ying Li deleted |
Priority: | high → highest |
os.fdopen
takes a mode, so some additional code generating a mode from the given flags was added (and also changed it so that if both flags and mode are given, an error is raised). This is to prevent the user from inputting incompatible mode and flags (ex. mode='r', flags=os.O_WRONLY) since thenos.fdopen
will raise a somewhat unhelpful error along the lines of "Invalid argument" or something of the sort. A bunch of tests to make sure that the modes were correctly generated, or at least that behavior is correct for given flags, were added. These tests call read/write on the file handle returned, so that basically also answers the question of whether what's returned is a valid file handle.
- test is renamed, and the other test (which was named
testOpen
) has been changed to have an underscore. I also named ittest_openWithMode
to differentiate it fromtest_openWithFlags
assert
has been removed, and aValueError
raised instead. I'm not sure if this is what you mean, but I did include a test for it intest_openWithMode
.
Changed 14 years ago by
Attachment: | filepath.open.flags.2.patch added |
---|
comment:4 Changed 14 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Ying Li |
- open is documented to raise
OSError
, but I think you meantValueError
- test_openWithFlags should be split in 9 or 10 smaller tests: basically, the comments you wrote are almost docstrings for the tests where they should belong.
Thanks!
comment:5 Changed 14 years ago by
Keywords: | review added |
---|---|
Owner: | Ying Li deleted |
Ok, changed both things. I was just following the style of the original testOpen (now test_openWithMode).
Changed 14 years ago by
Attachment: | filepath.open.flags.3.patch added |
---|
comment:8 Changed 14 years ago by
Owner: | set to therve |
---|---|
Priority: | highest → high |
comment:9 Changed 14 years ago by
author: | → therve |
---|---|
Branch: | → branches/filepath-open-flags-3123 |
(In [23190]) Branching to 'filepath-open-flags-3123'
comment:10 follow-up: 11 Changed 14 years ago by
Keywords: | review removed |
---|
What if FilePath
didn't try to be as smart here? It's okay for these APIs to be low level (consider, eg FilePath.chown
which uses -1
to indicate no-change :). Something to keep in mind is that someday there may be an IFilePath
and some of these methods may end up on it, and someone may have to implement them for some other backend. So simplicity in implementation is nice (particularly if the API is amenable to a higher-level API for the common base being implemented on top of it such that it will work with any implementation).
So do you think FilePath.open
could require mode
to be supplied if flags
is and just use what's specified, requiring it to be sensible? I haven't thought about this too much, this is just an initial reaction to the code. I'm open to the possibility that the current approach is better.
comment:11 Changed 14 years ago by
Keywords: | review added |
---|
As far as I can tell, there is only one mapping of flags to mode - it's not really "smart", it just saves the user some work because they'd have to write the same flag-to-mode mapping code if they wanted to use open
with flags anyway. That is, unless they're hardcoding in some flags, in which case I guess it would save them a few seconds of thinking and mentally mapping the flags onto a mode. I thought it would be better to provide the convenience rather than make anyone who wanted to open a FilePath
with flags to write their own version.
Also, the error returned by os.fdopen
when mode and flags don't match is pretty vague - it would make debugging for a user somewhat complicated. If a more helpful error were to be returned, error checking would need to be done using the same code anyway.
If your concern is that there may be an implementation of IFilePath
that needs completely different flags and modes... I submitted this patch because I am working on a FilePath based SFTP server, and I need to pass file transfer flags to open. These flags map onto the os file flags perfectly, and as it's a network protocol, it seems that the [read, write, readwrite, append, truncate, create] flags are universal enough. Do you think there should be a separate enumeration of universal flags in FilePath that should be used instead of the ones defined in the os module, so that the flags can be specified for different implementations of IFilePath?
Anyway, I appreciate the comment. :) It didn't seem like a review though, so did you delete the "review" keyword by accident? I notice that you didn't reassign it to me, which is why I ask. I put the review keyword back just in case - apologies if it was intentional.
comment:12 Changed 14 years ago by
I just dropped the review keyword to make sure I got your attention. I meant to re-assign it to you as well, but forgot. Sorry about that.
I'll think some more about the rest of your response (or maybe someone else will do a review).
comment:13 Changed 14 years ago by
Owner: | changed from therve to Jean-Paul Calderone |
---|
comment:14 follow-up: 20 Changed 14 years ago by
Cc: | Jean-Paul Calderone added |
---|---|
Keywords: | review removed |
Owner: | changed from Jean-Paul Calderone to Ying Li |
Priority: | high → normal |
Prior offline discussion went something like this:
Change the preferred signature of FilePath.open
from what it currently is to open(modeObject)
where modeObject
is an object with getModeString
and getModeFlags
methods (perhaps with better names). getModeString
returns a string like "wb+"
. getModeFlags
returns flags suitable for use with os.open
. If the mode string is sufficient to completely describe how to open the file, FilePath.open
can just use file
and return the result. If the flags are necessary, then os.open
is used, along with os.fdopen
to create a file object.
The current string-style parameter should remain supported, of course, for backwards compatibility.
comment:15 follow-up: 19 Changed 14 years ago by
Also, it occurs to me that most mode strings can be extrapolated from a set of mode flags. However, I don't think that "b" vs "t" can be. It might make sense to drop getModeString
in favor of getTextMode
so as to minimize the redundancy.
comment:16 Changed 11 years ago by
Owner: | Ying Li deleted |
---|
comment:17 Changed 11 years ago by
Owner: | set to Glyph |
---|---|
Status: | new → assigned |
comment:18 Changed 11 years ago by
Owner: | changed from Glyph to Ying Li |
---|---|
Status: | assigned → new |
comment:19 Changed 10 years ago by
Replying to exarkun:
Also, it occurs to me that most mode strings can be extrapolated from a set of mode flags.
Yes, but I wonder: is it actually wise to do that? http://pubs.opengroup.org/onlinepubs/009695399/functions/fopen.html makes no mention of flags, which makes me wonder if there is some buggy C library that we will need to work around in the future with some bonus non-standard mode string for which there is no equivalent mode flag...
(Obviously the IFilePath version shouldn't care about dumb stuff like that, but maybe FilePath
itself should allow the passthrough to be more direct? I don't know.)
However, I don't think that "b" vs "t" can be. It might make sense to drop
getModeString
in favor ofgetTextMode
so as to minimize the redundancy.
For what it's worth, as per http://docs.python.org/library/os.html#open-flag-constants 'b' vs. 't' can be described by O_BINARY
vs. O_TEXT
(which are only defined on Windows, but then, these flags only have an effect on Windows). But U
can't be.
comment:20 Changed 10 years ago by
Replying to exarkun:
Prior offline discussion went something like this:
Change the preferred signature of
FilePath.open
from what it currently is toopen(modeObject)
wheremodeObject
is an object withgetModeString
andgetModeFlags
methods (perhaps with better names).getModeString
returns a string like"wb+"
.getModeFlags
returns flags suitable for use withos.open
. If the mode string is sufficient to completely describe how to open the file,FilePath.open
can just usefile
and return the result. If the flags are necessary, thenos.open
is used, along withos.fdopen
to create a file object.
Reflecting on this, I don't like it very much now. Instead of something like this:
from twisted.python.filepath import FilePath from os import O_RDWR, O_CREAT # ... FilePath(foo).open(O_RDWR | O_CREAT)
with this proposal we'd have something like:
from twisted.python.filepath import FilePath, Pointless from os import O_RDWR, O_CREAT # ... FilePath(foo).open(Pointless(O_RDWR | O_CREAT))
Since the benefit of the latter proposal is not actually specified by the comment above, I'm assuming the goal is to make the implementation of FilePath.open
shorter (i.e. less smart), but that could be equally well accomplished by just making a modeStringFromFlags
function that tidies up the bit-twiddling out of the function itself.
Also, not clearly shown above is that if your function merely expects a FilePath
rather than constructing one, you will go from needing zero imports to use a mode string to needing imports from two different modules to use a mode ... object thing.
My current proposal is: FilePath.open(read=True, write=True, create=True, exclusive=True)
for the default case.
Then, perhaps a real-filesystem-only escape hatch so you can do platform-specific craziness like FilePath.open(read=True, write=True, create=True, extra=os.O_NOATIME)
if you really want to. (Separate ticket.)
That signature will make it a lot easier for e.g. ZipPath
to raise a sensible error in the write=True
case; it just has to do if write:
.
Better yet, I really like the idea of having a bunch of @param
s that describe how each flag actually works, instead of the conventional 'meh, you know how POSIX works, right' blow-off that both the standard library and our current documentation give.
Unfortunately, this is still slightly inconvenient for the original use-case; you can't pass flags directly from the wire through to os.open
. But then, as long as the features requested by the flags required by the standard are exposed by FilePath
, perhaps passing them direct from the wire is a bad thing. Wire clients probably shouldn't be able to pass O_DIRECT
, for example.
The current string-style parameter should remain supported, of course, for backwards compatibility.
For sure. Let's take care not to make the new, "good" version so much more inconvenient that everybody keeps wanting the compatibility version forever, though.
comment:21 Changed 10 years ago by
I might not have enough bandwidth to fully consider these most recent comments. However, one thing that did occur to me and which may be a good idea is that instead of FilePath.open(read=True, write=True, create=True, exclusive=True)
we could go with the result of #5384 for this instead of a bunch of booleans (maybe this is also ill conceived, but I functions that take sixteen booleans also don't strike me as super rad).
Added an additional optional "flags" parameter to
FilePath.open
- if it is not None, passes the flags straight to os.open, ignoring mode.