Fix shortcuts.run crashes in text mode in python3 #133
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JayZ12138
force-pushed
the
7875
branch
2 times, most recently
from
39f5f01 to
ee03831
Compare
Pull Request Test Coverage Report for Build 173
💛 - Coveralls |
JayZ12138
requested review from
lubomir,
rohanpm,
chandwanitulsi,
danrodrig,
jbruzl,
midnightercz,
negillett,
rajulkumar and
rbikar
In python3, some new args are introduced to Popen, which are 'text', 'encoding' and 'errors', when any of these args is set, Popen will enable the text mode, leading shortcuts.run to crash. This patch fix makes it recoginize these new args.
JayZ12138
changed the title
negillett
approved these changes
Dec 12, 2019
rohanpm
reviewed
Jan 7, 2020
kobo/shortcuts.py
Outdated
|
|
||
| # any of these args is passed, text mode will be enabled. | ||
| is_text_mode = any([universal_newlines, text, encoding, errors]) | ||
| encoding_method = encoding or 'utf-8' |
There was a problem hiding this comment.
To me it'd be cleaner to just reuse encoding variable, like:
Suggested change
| encoding_method = encoding or 'utf-8' | |
| encoding = encoding or 'utf-8' |
The encoding_method name is a little awkward, it's unclear that encoding and encoding_method really mean the same thing except that one of them is the raw value passed by the caller and the other is the value with some default possibly applied. How about just applying the default to encoding directly?
There was a problem hiding this comment.
yeah, makes sense.
rohanpm
approved these changes
Jan 7, 2020
chandwanitulsi
approved these changes
Jan 7, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In python3, some new args are introduced to Popen, which are 'text',
'encoding' and 'errors', when any of these args is set, Popen will
enable the text mode, leading shortcuts.run to crash.
This patch makes it recognize and be able to deal with these new args.