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.

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

STYLE: use types_or in pre-commit #38457

Merged
merged 4 commits into from Dec 16, 2020
Merged

STYLE: use types_or in pre-commit #38457

merged 4 commits into from Dec 16, 2020

Conversation

rkc007
Copy link
Contributor

@rkc007 rkc007 commented Dec 13, 2020

MarcoGorelli
MarcoGorelli requested changes Dec 14, 2020
View reviewed changes
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

Thanks @rkc007 ,

The idea isn't to replace types with types_or everywhere, but only where it makes things simpler, please see my comments

.pre-commit-config.yaml Outdated
@@ -24,7 +25,7 @@ repos:
name: isort (python)
- id: isort
Copy link
Member

Choose a reason for hiding this comment

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

The two isort hooks can be combined into one

Suggested change
- id: isort
- id: isort
types: [text]
types_or: [python, cython]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @MarcoGorelli for the explanation.

.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@rkc007
Copy link
Contributor Author

rkc007 commented Dec 14, 2020

Thanks, @MarcoGorelli for correcting my mistakes. I have fixed the PR. Can you please review it again?

MarcoGorelli
MarcoGorelli requested changes Dec 14, 2020
View reviewed changes
.pre-commit-config.yaml Outdated
- id: unwanted-patterns-strings-to-concatenate
name: Check for use of not concatenated strings
language: python
entry: python scripts/validate_unwanted_patterns.py --validation-type="strings_to_concatenate"
files: \.(py|pyx|pxd|pxi)$
types_or: [python, file, executable, text]
Copy link
Member

Choose a reason for hiding this comment

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

I think this will match too many files - it will match anything that is python or file or executable or text.

Instead, we only want \.(py|pyx|pxd|pxi)$ files. .py is Python. .pyx, .pxd and .pxi are all Cython:

(pandas-dev) marco@marco-Predator-PH315-52:~/pandas-dev$ identify-cli pandas/_libs/lib.pxd
["cython", "file", "non-executable", "text"]
(pandas-dev) marco@marco-Predator-PH315-52:~/pandas-dev$ identify-cli pandas/_libs/lib.pyx
["cython", "file", "non-executable", "text"]
(pandas-dev) marco@marco-Predator-PH315-52:~/pandas-dev$ identify-cli ./pandas/_libs/algos_take_helper.pxi
["cython", "executable", "file", "text"]

So here we want

types_or: [python, cython]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

.pre-commit-config.yaml Outdated
- id: unwanted-patterns-strings-with-wrong-placed-whitespace
name: Check for strings with wrong placed spaces
language: python
entry: python scripts/validate_unwanted_patterns.py --validation-type="strings_with_wrong_placed_whitespace"
files: \.(py|pyx|pxd|pxi)$
types_or: [python, file, executable, text]
Copy link
Member

Choose a reason for hiding this comment

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

same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

MarcoGorelli
MarcoGorelli reviewed Dec 14, 2020
View reviewed changes
.pre-commit-config.yaml Outdated
- id: isort
name: isort (cython)
types: [cython]
types: [text]
Copy link
Member

Choose a reason for hiding this comment

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

would also be good add a comment here, like

        types: [text]  # overwrite upstream `types: [python]`

I think such comments work in yaml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added!

@MarcoGorelli MarcoGorelli added the Code Style Code style, linting, code_checks label Dec 14, 2020
@MarcoGorelli
Copy link
Member

MarcoGorelli commented Dec 14, 2020 •
edited

Also, please update the minimum pre-commit version in environment.yml and in `requirements-dev.txt:

  - pre-commit>=2.9.2

MarcoGorelli
MarcoGorelli reviewed Dec 14, 2020
View reviewed changes
.pre-commit-config.yaml Outdated
@@ -1,3 +1,4 @@
minimum_pre_commit_version: '2.9.0'
Copy link
Member

Choose a reason for hiding this comment

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

let's make this 2.9.2, as the release note mentions

Fix default value for types_or so symlink and directory can be matched

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@rkc007
Copy link
Contributor Author

rkc007 commented Dec 14, 2020

Thank you so much for correcting my mistakes again. I am in process of understanding the whole pandas library. Thank you for your help :)
I have fixed it. Can you please review it again?

@MarcoGorelli MarcoGorelli changed the title ENH: Changed types to types_or for pre-commit STYLE: Changed types to types_or for pre-commit Dec 15, 2020
@MarcoGorelli MarcoGorelli changed the title STYLE: Changed types to types_or for pre-commit STYLE: use types_or in pre-commit Dec 15, 2020
MarcoGorelli
MarcoGorelli approved these changes Dec 15, 2020
View reviewed changes
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks @rkc007 !

@jreback jreback added this to the 1.3 milestone Dec 16, 2020
@jreback jreback merged commit af45b0a into pandas-dev:master Dec 16, 2020
@jreback
Copy link
Contributor

jreback commented Dec 16, 2020

thanks @rkc007

luckyvs1 pushed a commit to luckyvs1/pandas that referenced this pull request Jan 20, 2021
@rkc007 @luckyvs1
STYLE: use types_or in pre-commit (pandas-dev#38457)
2bede83
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarcoGorelli MarcoGorelli MarcoGorelli approved these changes

Assignees
No one assigned
Labels
Code Style Code style, linting, code_checks
Projects
None yet
Milestone
1.3
Development

Successfully merging this pull request may close these issues.

STYLE use types_or in pre-commit
3 participants
@rkc007 @MarcoGorelli @jreback