Skip to content

Conversation

@krajatcl
Copy link
Contributor

@krajatcl krajatcl commented Sep 27, 2020

…sets/test_ticks.py

  • closes #xxxx
  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

ref: #30999

@krajatcl
Copy link
Contributor Author

Please have look at this PR. I am new to this, I have tried to follow guidelines as much as possible.

FYI @MomIsBestFriend

@dsaxton dsaxton added the Testing pandas testing functions or related to the test suite label Sep 27, 2020
"cls, left, right, comparison",
[
(cls, left, right, comparison)
for cls in tick_classes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want for loops inside the parameters, parametrizing is already a type of for loop. If you check some of the other examples in the code base it might become a bit more clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i will have look at this. I used this as left, right arguments were dependent on cls vlaue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I missed that part, maybe parametrizing isn't as clean as I thought

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so should i remove it from left and right. And keep it only for cls and comparison.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now I suppose just adding that error message should be good for this PR (any other cleanup could always be done as follow-up). Sorry for the confusion.

Copy link
Contributor

@dsaxton dsaxton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @krajatcl

@dsaxton dsaxton merged commit 823f36b into pandas-dev:master Sep 30, 2020
kesmit13 pushed a commit to kesmit13/pandas that referenced this pull request Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Testing pandas testing functions or related to the test suite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants