On 9/25/20 9:26 AM, Eduardo Habkost wrote:
On Fri, Sep 25, 2020 at 03:15:57PM +0200, Markus Armbruster wrote:
Cleber Rosa <cr...@redhat.com> writes:

On Wed, Sep 23, 2020 at 02:37:27PM -0400, John Snow wrote:
On 9/23/20 11:26 AM, Eduardo Habkost wrote:
On Tue, Sep 22, 2020 at 05:00:51PM -0400, John Snow wrote:
Make the file handling here just a tiny bit more idiomatic.
(I realize this is heavily subjective.)

Use exist_ok=True for os.makedirs and remove the exception,
use fdopen() to wrap the file descriptor in a File-like object,
and use a context manager for managing the file pointer.

Signed-off-by: John Snow <js...@redhat.com>

Reviewed-by: Eduardo Habkost <ehabk...@redhat.com>

I really miss a comment below explaining why we use
open(os.open(pathname, ...), ...) instead of open(pathname, ...).

This code:

         fd = os.open(pathname, os.O_RDWR | os.O_CREAT, 0o666)
         f = open(fd, 'r+', encoding='utf-8')

Not known to me. It was introduced in 907b846653 as part of an effort to
reduce rebuild times. Maybe this avoids a modification time change if the
file already exists?

Markus?

AFACIT the change on 907b846653 is effective because of the "is new
text different from old text?" conditional.  I can not see how the
separate/duplicate open/fdopen would contribute to that.

But, let's hear from Markus.

This was my best attempt to open the file read/write, creating it if it
doesn't exist.

Plain

         f = open(pathname, "r+", encoding='utf-8')

fails instead of creates, and

         f = open(pathname, "w+", encoding='utf-8')

truncates.

If you know a better way, tell me!

Thanks for the explanation!

Yeah, it looks like there's no combination of open() flags that
would get translated to O_RDWR|O_CREAT.

Using os.open() like you did seems more straightforward than
catching FileNotFoundError.


OK. Using fdopen works for us, so let's stick with it. I will add a comment explaining our reasoning.

Thanks!

--js


Reply via email to