Skip to content

Conversation

@MarcoGorelli
Copy link
Member

Like this they're cross-platform, provide faster feedback to devs, and can use pre-commit pygrep instead of the custom invgrep

language: pygrep
entry: >-
(from pandas.core.common import|from pandas.core import common
|from collections.abc import|from numpy import nan)
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: id put each of these on separate lines, same on L76-77

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd got the multiline string wrong. >- will put a space between each line. Solution is to use " and to escape the backslashes (as in the latest commit)

@jbrockmendel
Copy link
Member

This looks fine. Has there been a discussion somewhere where theres consensus to move as much as possible/convenient to the pre-commit?

@MarcoGorelli
Copy link
Member Author

This looks fine. Has there been a discussion somewhere where theres consensus to move as much as possible/convenient to the pre-commit?

There hasn't, there was just this discussion as a precedent #36386 , and Will's comment

These in particular are managed through the configuration file, so they won't differ from being run in pre-commit which has the added bonus of being cross platform

There was one PR (#37136) where using pygrep instead of invgrep revealed a case which wasn't being found.

I'd welcome a discussion about whether this is welcome in general of course - as per this PR's description, I think it would be beneficial

@jreback jreback added CI Continuous Integration Code Style Code style, linting, code_checks labels Oct 20, 2020
@jreback jreback added this to the 1.2 milestone Oct 20, 2020
@jreback
Copy link
Contributor

jreback commented Oct 20, 2020

can you merge master and pin gon green.

@MarcoGorelli MarcoGorelli requested review from jreback and removed request for jreback October 20, 2020 09:31
@MarcoGorelli MarcoGorelli marked this pull request as draft October 20, 2020 12:01
@MarcoGorelli MarcoGorelli force-pushed the non-standard-imports branch 2 times, most recently from c801ff4 to 0d1f75d Compare October 20, 2020 13:25
@MarcoGorelli MarcoGorelli marked this pull request as ready for review October 20, 2020 13:31
@MarcoGorelli MarcoGorelli marked this pull request as draft October 20, 2020 15:03
@MarcoGorelli MarcoGorelli marked this pull request as ready for review October 20, 2020 16:39
@MarcoGorelli MarcoGorelli marked this pull request as draft October 20, 2020 16:41
@MarcoGorelli MarcoGorelli marked this pull request as ready for review October 20, 2020 16:42
@MarcoGorelli
Copy link
Member Author

can you merge master and pin gon green.

ping

- id: non-standard-imports-in-tests
name: Check for non-standard imports in test suite
language: pygrep
entry: |
Copy link
Contributor

Choose a reason for hiding this comment

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

as a followup we might want to update the contributing docs to put a pointer to these types of checks here (e.g. rather than just mentioning code_checks.sh) esp as we are using these a lot more now

@jreback jreback merged commit 1b7e3a6 into pandas-dev:master Oct 22, 2020
@jreback
Copy link
Contributor

jreback commented Oct 22, 2020

thanks @MarcoGorelli

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI Continuous Integration Code Style Code style, linting, code_checks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants