TST: Verify functioning of histtype argument (GH23992) #37063
Conversation
avinashpancham
force-pushed
the
gh23992
branch
from
db33849 to
17cfac2
Compare
|
@mroeschke you added the testing label to the original issue. Could you maybe have a look whether this test meets the standards? Thanks! |
mroeschke
added
Testing
|
Not as familiar with this part of the code base but tagged appropriately. |
There was a problem hiding this comment.
thanks @avinashpancham for the PR
i put a few comments for feedback
pandas/tests/plotting/common.py
Outdated
| @@ -173,6 +173,27 @@ def _check_visible(self, collections, visible=True): | |||
| for patch in collections: | |||
| assert patch.get_visible() == visible | |||
|
|
|||
| def _check_filled(self, ax, filled=True): | |||
There was a problem hiding this comment.
i would suggest to rename it, since _check_filled sounds a bit vague to me
also would be nice to annotate.
There was a problem hiding this comment.
Updated name to _check_filling.
What do you mean with annotate, since the function already has a docstring.
There was a problem hiding this comment.
Sorry my bad, I was bit confused wrt to annotation. Added annotation to all new functions
There was a problem hiding this comment.
emm, i meant to give it a more meaningful name than _check_filling, maybe use _check_patches_all_filled or _check_artists_all_filled?
There was a problem hiding this comment.
Renamed to _check_patches_all_filled
|
@charlesdong1991 I have some issues when adding the type annotations, since the CI produces the error below. Locally this works since I have installed matplotlib in my env. How should I add the type annotation if matplotlib is not available on he machine, cause at the moment I am a bit stuck. |
|
emm, maybe could you try this? from typing import TYPE_CHECKING
if TYPE_CHECKING:
from matplotlib.axes import Axes |
| ("stepfilled", True), | ||
| ], | ||
| ) | ||
| def test_histtype_argument(self, histtype: str, expected: bool) -> None: |
There was a problem hiding this comment.
pls remove annotations for tests.
maybe i wasn't clear enough, I only meant to add annotation for the helper function you add in common.py.
| ("stepfilled", True), | ||
| ], | ||
| ) | ||
| def test_histtype_argument(self, histtype: str, expected: bool) -> None: |
There was a problem hiding this comment.
the same here and below
|
CI encountered "RuntimeError: can't start new thread". Would it be possible to restart the CI? Thanks! |
|
@charlesdong1991 updated the code, merged master and CI is on green. Could you review it again? |
|
@charlesdong1991 over to you, merge when good |
|
Sorry I saw I forgot to rename function |
|
OK, everything is done now :) |
) * TST: Verify functioning of histtype argument (GH23992) * Generalize checks and store in common.py * Update _check_filling function * Add type hinting * Update Axis reference to be in line with plotting/_matplotlib/tools.py * Add conditional import of matplotlib * Remove type hinting from tests * Rename function as requested
) * TST: Verify functioning of histtype argument (GH23992) * Generalize checks and store in common.py * Update _check_filling function * Add type hinting * Update Axis reference to be in line with plotting/_matplotlib/tools.py * Add conditional import of matplotlib * Remove type hinting from tests * Rename function as requested
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diff