Skip to content

bpo-27300: Add missing error= argument to tempfile classes#6696

Merged
serhiy-storchaka merged 8 commits into
python:masterfrom
sth:tempfile-error-parameter
May 23, 2018
Merged

bpo-27300: Add missing error= argument to tempfile classes#6696
serhiy-storchaka merged 8 commits into
python:masterfrom
sth:tempfile-error-parameter

Conversation

@sth

@sth sth commented May 3, 2018

Copy link
Copy Markdown
Contributor

The classes for temporary files in tempfile have an encoding parameter to specify the file encoding. They don't have an error parameter to specify what should happen on encoding errors. This is unusual, normally things with encoding also have errors.

This pull request adds the missing errors parameter. It is treated the same as the already existing encoding parameter: the parameter is simply handed through to the underlying io class.

https://bugs.python.org/issue27300

@serhiy-storchaka

Copy link
Copy Markdown
Member

Since the order of parameters doesn't match the order of corresponding parameters of io.open(), I suggest to make errors a keyword-only parameter. In future we may make all parameters except the first one a keyword-only, but this is a different issue.

@sth

sth commented May 5, 2018

Copy link
Copy Markdown
Contributor Author

That's a good idea, I changed errors accordingly.

Comment thread Lib/tempfile.py Outdated
try:
return _io.open(fd, mode, buffering=buffering,
newline=newline, encoding=encoding)
newline=newline, encoding=encoding, errors=errors)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too long line.

Comment thread Lib/tempfile.py Outdated
def errors(self):
try:
return self._file.errors
except AttributeError:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it needed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accessing self._file.errors might raise AttributeError because in this class _file is initially a StringIO or BytesIO instance. The workaround with catching AttributeError is maybe not optimal, but that's the way it's done for the other file attributes (like encoding, mode, ...).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but StringIO has the errors attribute, and in the case of BytesIO the exception will be reraised.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't even aware that StringIO has this attribute :). It does indeed work the same without the try.

@@ -0,0 +1,2 @@
The file classes in *tempfile* now accept an *errors* parameter that
complements the already existing *encoding*.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add "Patch by yourname."

@sth

sth commented May 20, 2018

Copy link
Copy Markdown
Contributor Author

The existing properties encoding and newlines also contain useless exception handling (Like for the errors property discussed in the above diff). This should probably be simplified the same way as for errors.

Should I create a separate PR for that?

@serhiy-storchaka

Copy link
Copy Markdown
Member

It is good if clean up that code in this PR.

@serhiy-storchaka serhiy-storchaka merged commit 825aab9 into python:master May 23, 2018
@serhiy-storchaka serhiy-storchaka added the type-feature A feature request or enhancement label May 23, 2018
@sth sth deleted the tempfile-error-parameter branch June 6, 2018 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type-feature A feature request or enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants