Skip to content
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.

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

TST: Add docstrings to arithmetic fixtures #29441

Merged
merged 6 commits into from
Nov 18, 2019

Conversation

dalgarno
Copy link
Contributor

@dalgarno dalgarno commented Nov 6, 2019

Relates to #19159

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

@jreback jreback added the Testing pandas testing functions or related to the test suite label Nov 6, 2019
@dalgarno dalgarno marked this pull request as ready for review November 6, 2019 20:41
Copy link
Member

@datapythonista datapythonista left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @dalgarno.

This looks like an improvement, but after reading the docstrings it's still not clear what those fixture do, or how they should be used. Do you mind trying to rephrase, and may be use examples?

@datapythonista datapythonista changed the title Add docstrings to arithmetic fixtures TST: Add docstrings to arithmetic fixtures Nov 6, 2019
Several types of scalar zeros and length 5 vectors of zeros. For
testing division by (or of) zero.

Uses vector of length 5 for broadcasting with `numeric_idx` fixture,
Copy link
Member

Choose a reason for hiding this comment

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

Can you model the extended description here after what you have in the other method? I find that quite nice, but don't really understand this one

@simonjayhawkins simonjayhawkins added this to the 1.0 milestone Nov 8, 2019
@pep8speaks
Copy link

pep8speaks commented Nov 12, 2019

Hello @dalgarno! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-11-12 21:52:27 UTC

@jreback
Copy link
Contributor

jreback commented Nov 13, 2019

lgtm. side issue we should rename these to maybe one_valueand so on and move to pandas/conftest.py (but for another PR); also I believe we have multiple copies of these fixtures in various locations.

@dalgarno
Copy link
Contributor Author

@WillAyd can you approve if you're happy?

Copy link
Member

@datapythonista datapythonista left a comment

Choose a reason for hiding this comment

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

lgtm, thanks @dalgarno

@WillAyd WillAyd merged commit 527143b into pandas-dev:master Nov 18, 2019
@WillAyd
Copy link
Member

WillAyd commented Nov 18, 2019

Thanks @dalgarno

Reksbril pushed a commit to Reksbril/pandas that referenced this pull request Nov 18, 2019
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
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.

6 participants