7 ms·

The point of a file context manager is to close the file when you're done with it. The CSV reader isn't done with it. Given that the CSV reader does not contro

The point of a file context manager is to close the file when you're done with it. The CSV reader isn't done with it. Given that the CSV reader does not control ownership of the file handle, it shouldn't return a lazy iterator when given a file handle. It should accept a path, then it could own the file handle until the iterator is complete or dropped. So, yea, the CSV reader does a bad. But so what? That is a deficiency of the CSV library, not context managers.

dmurray 6y ago
The CSV library is part of the standard library, though. Its way of doing things has been blessed by the Python maintainers. OK, it might still be correct to say the problem is with the CSV library. But if the context manager "best practices" lead to popular parts of the standard library being bad and unidiomatic, that's at best unpythonic. After all, "There should be one -- and preferably only one -- obvious way to do it".
- arugulum 6y ago
  By the Python standard library already does contain things that don't follow best practice. See: logging doesn't abide by PEP 8.
throwaway894345 6y ago
Libraries that accept filepaths (except perhaps as a convenience) are bad. The CSV library is well-designed. It isn’t responsible for closing files because it doesn’t take files, it takes TextIO. None of this is to say context managers are bad; I think they’re great but they only work as intended.
- solidasparagus 6y ago
  That's being a little pedantic IMO. From a practical, user-facing perspective, the CSV library Reader takes in files. And they know this, which is why the documentation specifically calls out gotchas related to passing in a file object and the example code shows how it should work with a file object [1]. [1] https://docs.python.org/3/library/csv.html#csv.reader https://docs.python.org/3/library/csv.html#csv.reader
  - kevin_thibedeau 6y ago
    File like objects are not necessarily files. A library consuming them can't presume to manage them.
  - throwaway894345 6y ago
    It’s not pedantic. It’s a very important distinction for exactly this use case. The CSV library doesn’t and shouldn’t close the source object because it may not have a close method and even if it does, someone else might be planning on closing it later (the scope that owns it) which might be an error depending on the implementation of the source’s close() method. Only the resource owner should close it, and the CSV library shouldn’t be the owner.
  - anang 6y ago
    I don’t think it’s at all pedantic, the library doesn’t accept a path and by extension is one step of abstraction away from a file. I’ve used the CSV library many times with an in memory TextIO object (like an http response). It’s an important distinction.
- twic 6y ago
  > It isn’t responsible for closing files because it doesn’t take files, it takes TextIO. TextIO has a close() method, so this is irrelevant. Rather, the author of the CSV library made a design decision about whether a CSV reader should take ownership of the underlying IO or not. If it takes ownership, it has the responsibility of closing the file when it's finished. If it doesn't, it doesn't. The decision was made for it not to take ownership. Was this a good decision? Personally, i don't think so. There is nothing useful you can do with an IO object once a CSV reader has used it, so there is no point retaining ownership separately. It just makes more work for the programmer. This is compounded by the fact that there is no way to obtain the underlying IO object from a CSV reader, so you have to hold on to the files separately. In the normal use case where you open a single file and then read from it in a single block of code, this is not a big deal, because you can wrap that in a with block for the file. But as the author points out, it's a serious impediment to using lazy readers. I have also run into this problem when managing multiple open files; i have a tool which opens a bunch of CSV files and merges them in some strange way, and it is sensible to close each file when it is finished, but to do that, i need to hang on to the files, and build some machinery to close the right one at the right time. On the other hand, i can't think of any downsides to deciding, way back when, that CSV readers should close their underlying files. But it's not safe to change that decision now, so we're stuck with it.
  - masklinn 6y ago
    > Was this a good decision? Personally, i don't think so. I think the opposite. I've had issues with TextIOWrapper because it does (unexpectedly) take ownership of the IO object you give it, and closes it. Learned that the hard way, and had to replace TextIOWrapper by `codecs` reader and writer. > There is nothing useful you can do with an IO object once a CSV reader has used it You can seek the data back to the start to re-read it differently. Though I'll acknowledge that it's significantly more useful for a csv writer (and then if reader and writer diverge you've created an inconvenient asymmetry). > This is compounded by the fact that there is no way to obtain the underlying IO object from a CSV reader, so you have to hold on to the files separately. Not exactly a difficult feat.
dragonwriter 6y ago
> So, yea, the CSV reader does a bad. But so what? That is a deficiency of the CSV library No, it's not. It's a deficiency of the proposed use of the CSV library, which is just dumb. Leaving a dangling external resource open isn't an irrelevant implementation detail.
Zanni 6y ago
The problem here isn't the CSV library, it's that the Load function divorces the context manager from the actual processing context. It's a bad abstraction.
Someone 6y ago
Indeed. I think the API should be either: def load(filepath): return csv.reader(filepath) def main: ... with load(myFilepath) as reader: for item in reader: ... where the CSV reader both opens and closes the file, or def load(file): return csv.reader(file) def main: ... with open(myFilepath) as file for item in load(file): ... where the CSV reader is ignorant about opening or closing anything. If desired, add a method to the first API that takes an open file, but don’t close it if callers do with load(myOpenFile) as reader: ...

dmurray 6y ago

The CSV library is part of the standard library, though. Its way of doing things has been blessed by the Python maintainers. OK, it might still be correct to say the problem is with the CSV library. But if the context manager "best practices" lead to popular parts of the standard library being bad and unidiomatic, that's at best unpythonic. After all, "There should be one -- and preferably only one -- obvious way to do it".

arugulum 6y ago
By the Python standard library already does contain things that don't follow best practice. See: logging doesn't abide by PEP 8.

throwaway894345 6y ago

Libraries that accept filepaths (except perhaps as a convenience) are bad. The CSV library is well-designed. It isn’t responsible for closing files because it doesn’t take files, it takes TextIO. None of this is to say context managers are bad; I think they’re great but they only work as intended.

solidasparagus 6y ago
That's being a little pedantic IMO. From a practical, user-facing perspective, the CSV library Reader takes in files. And they know this, which is why the documentation specifically calls out gotchas related to passing in a file object and the example code shows how it should work with a file object [1]. [1] https://docs.python.org/3/library/csv.html#csv.reader https://docs.python.org/3/library/csv.html#csv.reader
- kevin_thibedeau 6y ago
  File like objects are not necessarily files. A library consuming them can't presume to manage them.
- throwaway894345 6y ago
  It’s not pedantic. It’s a very important distinction for exactly this use case. The CSV library doesn’t and shouldn’t close the source object because it may not have a close method and even if it does, someone else might be planning on closing it later (the scope that owns it) which might be an error depending on the implementation of the source’s close() method. Only the resource owner should close it, and the CSV library shouldn’t be the owner.
- anang 6y ago
  I don’t think it’s at all pedantic, the library doesn’t accept a path and by extension is one step of abstraction away from a file. I’ve used the CSV library many times with an in memory TextIO object (like an http response). It’s an important distinction.
twic 6y ago
> It isn’t responsible for closing files because it doesn’t take files, it takes TextIO. TextIO has a close() method, so this is irrelevant. Rather, the author of the CSV library made a design decision about whether a CSV reader should take ownership of the underlying IO or not. If it takes ownership, it has the responsibility of closing the file when it's finished. If it doesn't, it doesn't. The decision was made for it not to take ownership. Was this a good decision? Personally, i don't think so. There is nothing useful you can do with an IO object once a CSV reader has used it, so there is no point retaining ownership separately. It just makes more work for the programmer. This is compounded by the fact that there is no way to obtain the underlying IO object from a CSV reader, so you have to hold on to the files separately. In the normal use case where you open a single file and then read from it in a single block of code, this is not a big deal, because you can wrap that in a with block for the file. But as the author points out, it's a serious impediment to using lazy readers. I have also run into this problem when managing multiple open files; i have a tool which opens a bunch of CSV files and merges them in some strange way, and it is sensible to close each file when it is finished, but to do that, i need to hang on to the files, and build some machinery to close the right one at the right time. On the other hand, i can't think of any downsides to deciding, way back when, that CSV readers should close their underlying files. But it's not safe to change that decision now, so we're stuck with it.
- masklinn 6y ago
  > Was this a good decision? Personally, i don't think so. I think the opposite. I've had issues with TextIOWrapper because it does (unexpectedly) take ownership of the IO object you give it, and closes it. Learned that the hard way, and had to replace TextIOWrapper by `codecs` reader and writer. > There is nothing useful you can do with an IO object once a CSV reader has used it You can seek the data back to the start to re-read it differently. Though I'll acknowledge that it's significantly more useful for a csv writer (and then if reader and writer diverge you've created an inconvenient asymmetry). > This is compounded by the fact that there is no way to obtain the underlying IO object from a CSV reader, so you have to hold on to the files separately. Not exactly a difficult feat.

dragonwriter 6y ago

> So, yea, the CSV reader does a bad. But so what? That is a deficiency of the CSV library No, it's not. It's a deficiency of the proposed use of the CSV library, which is just dumb. Leaving a dangling external resource open isn't an irrelevant implementation detail.

Zanni 6y ago

The problem here isn't the CSV library, it's that the Load function divorces the context manager from the actual processing context. It's a bad abstraction.

Someone 6y ago

Indeed. I think the API should be either: def load(filepath): return csv.reader(filepath) def main: ... with load(myFilepath) as reader: for item in reader: ... where the CSV reader both opens and closes the file, or def load(file): return csv.reader(file) def main: ... with open(myFilepath) as file for item in load(file): ... where the CSV reader is ignorant about opening or closing anything. If desired, add a method to the first API that takes an open file, but don’t close it if callers do with load(myOpenFile) as reader: ...