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

CLN/TST: normalize test_frame_apply #40113

Merged

Conversation

rhshadrach
Copy link
Member

  • Ensure all linting tests pass, see here for how to run them

Some minor normalizations before breaking up or parametrizing these tests

@rhshadrach rhshadrach added Testing pandas testing functions or related to the test suite Clean Apply Apply, Aggregate, Transform, Map labels Feb 27, 2021
@jreback jreback added this to the 1.3 milestone Feb 27, 2021
@jbrockmendel
Copy link
Member

@jorisvandenbossche there's an ArrayManager test currently failing on master; any obvious candidates for something recently-merged that might account for that?

@rhshadrach
Copy link
Member Author

@jbrockmendel @jorisvandenbossche - Just looking at recent commits, the earliest one I see failing is #40050. This failed in the PR as well. Both as well as this PR have a single test failure, test_groupby_dtype_inference_empty.

@jorisvandenbossche
Copy link
Member

Yes, that PR was simply merged with failing tests .. Merging master will resolve it now thanks to Brock

Comment on lines 1646 to 1650
# https://github.com/pandas-dev/pandas/issues/35940
# GH 35940
Copy link
Member

Choose a reason for hiding this comment

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

I personally don't find this an improvement. I know we are inconsistent about it, but if there is a full link, it's much easier to go to the issue

Copy link
Contributor

Choose a reason for hiding this comment

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

this is how we do for every other comment about the issue number

if we want to require a full link it's possible but would need a precommit hook

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 do that for "every other comment". We currently use the full link in 400 cases.

Not every detail needs to be controlled with a pre-commit hook

Copy link
Contributor

Choose a reason for hiding this comment

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

and how many w/o a link? i don't know but i would say it's way higher

we have to have a standard and then enforce it

we simply cannot track stylistic things any other way or involve personal preference here (once we agree on a standard)

Copy link
Member Author

Choose a reason for hiding this comment

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

@jorisvandenbossche - Thanks for the feedback here, I didn't realize there wasn't a consensus. I actually agree on the preference for a link (but also more strongly prefer consistency). I've reverted these changes.

Assuming we can get a consensus, if things were made consistent and we updated the dev docs, it seems to me that consistency would become the norm and not require enforcement after a short amount of time. If it doesn't turn out to be the case and becomes a hassle, we could then implement a pre-commit hook.

@jreback jreback merged commit c5d0d13 into pandas-dev:master Mar 3, 2021
@rhshadrach rhshadrach deleted the test_frame_apply_normalization branch March 3, 2021 03:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Apply Apply, Aggregate, Transform, Map Clean Testing pandas testing functions or related to the test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants