-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG: Allow IOErrors when attempting to retrieve default client encoding. #21531
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 18 commits
6c2987d
c31b914
510f4b1
1afda5f
b37cb75
0b1ca1d
bf26adb
77a95d2
2598595
59e0993
8b51b34
984da5c
4790e10
6156825
fb1dc2e
7cedfdc
35c0b3b
49ee7b5
40b3588
5e3c8af
f485223
ab67024
81547da
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
import pytest | ||
|
||
from pandas.io.formats.console import detect_console_encoding | ||
|
||
|
||
class MockEncoding(object): # TODO(py27): replace with mock | ||
""" | ||
Used to add a side effect when accessing the 'encoding' property. If the | ||
side effect is a str in nature, the value will be returned. Otherwise, the | ||
side effect should be an exception that will be raised. | ||
""" | ||
def __init__(self, encoding): | ||
super(MockEncoding, self).__init__() | ||
self.val = encoding | ||
|
||
@property | ||
def encoding(self): | ||
return raise_or_return(self.val) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current LINT error is because you are missing a blank line here |
||
|
||
def raise_or_return(val): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this be moved as an instance method of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm also using it down below to test when defaulting to system default -- it covers the same goal of throwing an exception like above here. I could make it a static method of MockEncoding, but I thought using that object for both cases wouldn't make sense since the encoding property is only being consumed from accesses by stdout or stdin. What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @WillAyd did you see this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's make this a class method just so all of the relevant constructs stay together |
||
if isinstance(val, str): | ||
return val | ||
else: | ||
raise val | ||
|
||
|
||
@pytest.mark.parametrize('empty,filled', [ | ||
['stdin', 'stdout'], | ||
['stdout', 'stdin'] | ||
]) | ||
def test_detect_console_encoding_from_stdout_stdin(monkeypatch, empty, filled): | ||
# Ensures that when sys.stdout.encoding or sys.stdin.encoding is used when | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can add a reference to the issue for this and the other new tests, i.e. |
||
# they have values filled. | ||
# GH 21552 | ||
with monkeypatch.context() as context: | ||
context.setattr('sys.{}'.format(empty), MockEncoding('')) | ||
context.setattr('sys.{}'.format(filled), MockEncoding(filled)) | ||
assert detect_console_encoding() == filled | ||
|
||
|
||
@pytest.mark.parametrize('encoding', [ | ||
MockEncoding(AttributeError), | ||
MockEncoding(IOError), | ||
MockEncoding('ascii') | ||
]) | ||
def test_detect_console_encoding_fallback_to_locale(monkeypatch, encoding): | ||
# GH 21552 | ||
with monkeypatch.context() as context: | ||
context.setattr('locale.getpreferredencoding', lambda: 'foo') | ||
context.setattr('sys.stdout', encoding) | ||
assert detect_console_encoding() == 'foo' | ||
|
||
|
||
@pytest.mark.parametrize('std,locale', [ | ||
[MockEncoding('ascii'), lambda: 'ascii'], | ||
[MockEncoding('ascii'), lambda: raise_or_return(Exception)], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of mixing usage of |
||
[MockEncoding(AttributeError), lambda: 'ascii'], | ||
[MockEncoding(AttributeError), lambda: raise_or_return(Exception)], | ||
[MockEncoding(IOError), lambda: 'ascii'], | ||
[MockEncoding(IOError), lambda: raise_or_return(Exception)] | ||
]) | ||
def test_detect_console_encoding_fallback_to_default(monkeypatch, std, locale): | ||
# When both the stdout/stdin encoding and locale preferred encoding checks | ||
# fail (or return 'ascii', we should default to the sys default encoding. | ||
# GH 21552 | ||
with monkeypatch.context() as context: | ||
context.setattr('locale.getpreferredencoding', locale) | ||
context.setattr('sys.stdout', std) | ||
context.setattr('sys.getdefaultencoding', lambda: 'sysDefaultEncoding') | ||
assert detect_console_encoding() == 'sysDefaultEncoding' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe move to 0.23.5? @jreback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a regression? If not, I would probably stay with
0.24.0
unless there is a compelling reason to backport-patch this bug.