BUG: Let CategoricalIndex take CategoricalDtype as dtype argument #18116
Conversation
|
lgtm. can you add a release note, 0.21.1 is good. |
topper-123
force-pushed
the
CategoricalIndex_dtype
branch
2 times, most recently
from
350e0be to
6d48a08
Compare
| tm.assert_index_equal(result, expected, exact=True) | ||
|
|
||
| # error to combine categories or ordered and dtype keywords args | ||
| with pytest.raises(ValueError): |
There was a problem hiding this comment.
We generally try to avoid using pytest.raises because tm.assert_raises_regex is more effective. What's the error message that you get when calling the constructor as such?
There was a problem hiding this comment.
pytest.raises is ok, sure if you really want to check the message then use assert_raises_regex, i don't think its a big deal here
There was a problem hiding this comment.
The message is "Cannot specify both dtype and categories or ordered." in both cases.
FYI, pytest can now take a parameter match which does the same thing (i.e. with pytest.raises(ValueError, match="Cannot specify both `dtype` and `categories` or `ordered`." ):). This is a new feature added in pytest 3.1, so its very new. Pandas doesn't seem to have a minimum specified version of pytest, but it's reasonable to be able to test pandas with version < 3.1 of pytest, though.
There was a problem hiding this comment.
actually we do have a min version of 3.1
There was a problem hiding this comment.
it’s in our travis install, the requirements files and i believe the docs
There was a problem hiding this comment.
Ok, found it in contributing.rst: "The earliest supported pytest version is 3.1.0.".
It would make sene to have it in install.rst also as there is a section there on running the test suite.
There was a problem hiding this comment.
the section in install should simply point to contributing
we don’t wants things in more than one place
There was a problem hiding this comment.
@jreback : Do you think it would make sense to no longer tm.assert_raises_regex if we have pytest.raises in our toolbox now (unrelated to PR)?
There was a problem hiding this comment.
i don’t see a lot of benefit of changing
but could be persuaded
topper-123
force-pushed
the
CategoricalIndex_dtype
branch
from
6d48a08 to
016f08c
Compare
Codecov Report
@@ Coverage Diff @@
## master #18116 +/- ##
==========================================
+ Coverage 91.25% 91.26% +<.01%
==========================================
Files 163 163
Lines 50124 50124
==========================================
+ Hits 45742 45745 +3
+ Misses 4382 4379 -3
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #18116 +/- ##
==========================================
+ Coverage 91.25% 91.26% +<.01%
==========================================
Files 163 163
Lines 50124 50124
==========================================
+ Hits 45742 45745 +3
+ Misses 4382 4379 -3
Continue to review full report at Codecov.
|
|
Pinging at green @jreback. |
|
thanks! |
(cherry picked from commit 58c2f09)
(cherry picked from commit 58c2f09)
git diff upstream/master -u -- "*.py" | flake8 --diffThis PR allows CategoricalIndex to take CategoricalDtype as its dtype argument, see #18109 for details.