Skip to content

Conversation

@h-vetinari
Copy link
Contributor

Follow-up to #22733, where some things were left undone to ease reviewing.

Mainly:

@pep8speaks
Copy link

Hello @h-vetinari! Thanks for submitting the PR.

@codecov
Copy link

codecov bot commented Oct 6, 2018

Codecov Report

Merging #23016 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #23016   +/-   ##
=======================================
  Coverage   92.19%   92.19%           
=======================================
  Files         169      169           
  Lines       50835    50835           
=======================================
  Hits        46868    46868           
  Misses       3967     3967
Flag Coverage Δ
#multiple 90.61% <ø> (ø) ⬆️
#single 42.35% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5551bcf...4dd7f90. Read the comment docs.

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

looks ok, if you can add some doc-strings would be great.

has_numeric_only=False, check_dtype=True,
check_dates=False, check_less_precise=False,
skipna_alternative=None):
def assert_stat_op_calc(opname, alternative, main_frame, has_skipna=True,
Copy link
Contributor

Choose a reason for hiding this comment

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

main_frame -> frame

def assert_stat_op_calc(opname, alternative, main_frame, has_skipna=True,
check_dtype=True, check_dates=False,
check_less_precise=False, skipna_alternative=None):

Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a doc-string describing things

@gfyoung gfyoung added Testing pandas testing functions or related to the test suite Clean labels Oct 7, 2018
skipna_alternative=None):

f = getattr(main_frame, name)
def assert_stat_op_calc(opname, alternative, frame, has_skipna=True,
Copy link
Member

@gfyoung gfyoung Oct 7, 2018

Choose a reason for hiding this comment

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

  • Just curious, why the name change?
  • In general, I believe we keep the underscore unless we intend to import this function into other test modules (which we aren't?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gfyoung
It came out of review from @jreback #22733 (comment):

can this just be a module level function now? maybe rename to assert_stat_ops

I then split it into _calc and _api. Happy to add an underscore or rename it to whatever.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha. My personal preference is underscore for the name given the reasoning I described above. The changes are good otherwise.

@jreback jreback added this to the 0.24.0 milestone Oct 7, 2018
@jreback jreback merged commit af271ba into pandas-dev:master Oct 7, 2018
@jreback
Copy link
Contributor

jreback commented Oct 7, 2018

thanks

@h-vetinari h-vetinari deleted the tst_frame_analytics branch October 8, 2018 17:08
tm9k1 pushed a commit to tm9k1/pandas that referenced this pull request Nov 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Clean Testing pandas testing functions or related to the test suite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants