Raise OptionError instead of KeyError in __getattr__. Fixes #19789. #19790
Conversation
|
Hello @jayfoad! Thanks for updating the PR. Cheers ! There are no PEP8 issues in this Pull Request. 🍻 Comment last updated on February 24, 2018 at 15:07 Hours UTC |
Codecov Report
@@ Coverage Diff @@
## master #19790 +/- ##
=========================================
Coverage ? 91.58%
=========================================
Files ? 150
Lines ? 48911
Branches ? 0
=========================================
Hits ? 44796
Misses ? 4115
Partials ? 0
Continue to review full report at Codecov.
|
pandas/core/config.py
Outdated
| @@ -196,7 +196,10 @@ def __getattr__(self, key): | |||
| if prefix: | |||
| prefix += "." | |||
| prefix += key | |||
| v = object.__getattribute__(self, "d")[key] | |||
| d = object.__getattribute__(self, "d") | |||
| if key not in d: | |||
There was a problem hiding this comment.
slightly more idiomatic to use a try/except here (to catch the KeyError) and raise the OptionError
jreback
added
Error Reporting
doc/source/whatsnew/v0.23.0.txt
Outdated
| @@ -876,3 +876,4 @@ Other | |||
| ^^^^^ | |||
|
|
|||
| - Improved error message when attempting to use a Python keyword as an identifier in a ``numexpr`` backed query (:issue:`18221`) | |||
| - Bug in :func:`pandas.core.DictWrapper.__getattr__` when looking up a non-existent key (:issue:`19789`) | |||
There was a problem hiding this comment.
Could you describe the bug a bit? Mention it raising a KeyError instead of the correct OptionError.
| @@ -428,3 +428,7 @@ def test_option_context_scope(self): | |||
|
|
|||
| # Ensure the current context is reset | |||
| assert self.cf.get_option(option_name) == original_value | |||
|
|
|||
| def test_dictwrapper_getattr(self): | |||
| options = self.cf.options | |||
There was a problem hiding this comment.
use an with pytest.raises here to check for the OptionError
There was a problem hiding this comment.
As well as the hasattr test or instead of it? hasattr already tests for AttributeError.
| @@ -428,3 +428,7 @@ def test_option_context_scope(self): | |||
|
|
|||
| # Ensure the current context is reset | |||
| assert self.cf.get_option(option_name) == original_value | |||
|
|
|||
| def test_dictwrapper_getattr(self): | |||
| options = self.cf.options | |||
There was a problem hiding this comment.
add the issue number as a comment here
|
thanks! |
git diff upstream/master -u -- "*.py" | flake8 --diff