Skip to content

TST: insert 'match' to bare pytest raises in pandas/tests/tseries/off… #36682

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 30, 2020
Merged

TST: insert 'match' to bare pytest raises in pandas/tests/tseries/off… #36682

merged 1 commit into from
Sep 30, 2020

Conversation

krajatcl
Copy link
Contributor

@krajatcl krajatcl commented Sep 27, 2020 •
edited
Loading

…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 reviewed Sep 27, 2020
View reviewed changes
@dsaxton dsaxton added the Testing pandas testing functions or related to the test suite label Sep 27, 2020
jbrockmendel
jbrockmendel reviewed Sep 29, 2020
View reviewed changes
dsaxton
dsaxton reviewed Sep 29, 2020
View reviewed changes
pandas/tests/tseries/offsets/test_ticks.py Outdated
"cls, left, right, comparison",
[
(cls, left, right, comparison)
for cls in tick_classes
Copy link
Member

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
Member

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
Member

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.

dsaxton
dsaxton approved these changes Sep 30, 2020
View reviewed changes
Copy link
Member

@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
@krajatcl
TST: insert 'match' to bare pytest raises in pandas/tests/tseries/off…
bb9a3ef 
…sets/test_ticks.py (pandas-dev#36682)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbrockmendel jbrockmendel jbrockmendel left review comments

@dsaxton dsaxton dsaxton approved these changes

Assignees
No one assigned
Labels
Testing pandas testing functions or related to the test suite
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@krajatcl @dsaxton @jbrockmendel