Skip to content

BLD: Run flake8 check on Cython files in pre-commit #30847

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

Merged
merged 6 commits into from
Feb 12, 2020

Conversation

xhochy
Copy link
Contributor

@xhochy xhochy commented Jan 9, 2020

.flake8.cython Outdated
@@ -0,0 +1,3 @@
[flake8]
filename = *.pyx,*.pxd,*.pxi
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if it's a typo or it's intentional, but we used to validate only pyx files for these errors, and the CI is failing because there are pxd and pxi files that got errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this is a typo. I separated it out to another .flake8, pushed the fix.

@datapythonista datapythonista added CI Continuous Integration Code Style Code style, linting, code_checks labels Jan 9, 2020
@xhochy
Copy link
Contributor Author

xhochy commented Jan 9, 2020

@datapythonista Fixed the configuration, CI now passes.

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.

Looks good, I'd probably have the flake config files somewhere else, since we've already got too much in our root, but other than that looks good. I'll let someone else to review, since I'm not using pre-config myself.

@TomAugspurger if you're not busy with the RC, I guess you may want to have a look here.

@gfyoung
Copy link
Member

gfyoung commented Jan 10, 2020

Looks good, I'd probably have the flake config files somewhere else, since we've already got too much in our root, but other than that looks good.

I think a flake8 directory would make sense. Would also help to simplify filenames.

@xhochy
Copy link
Contributor Author

xhochy commented Jan 11, 2020

flake8 or .flake8?

@gfyoung
Copy link
Member

gfyoung commented Jan 11, 2020

flake8 (no dot) should be fine I think. Keep things simple.

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

@@ -0,0 +1,4 @@
[flake8]
filename = *.pxd,*.pxi.in
Copy link
Member

Choose a reason for hiding this comment

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

Can the template files go in with the normal pyx files? I think they would be closer to that than the header files

Copy link
Member

Choose a reason for hiding this comment

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

@xhochy can you check this? I think good to go otherwise

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pxd can go with the normal pyx files but the pxi.in raise a lot of errors, especially with indentation. I guess that the current indentation is actually sometimes preferred to the flake8 one as it makes the templates more readable.

@jreback
Copy link
Contributor

jreback commented Feb 9, 2020

@jbrockmendel can you try this out

@jbrockmendel
Copy link
Member

I'm missing something from the workflow. I've checked out this branch, added a flake-violation to groupby.pyx, but commiting it doesnt seem to cause any problem. What am I missing?

Can W293 for trailing whitespace be added?

Not a deal-breaker, but I like the more verbose way we list codes in statsmodels: https://github.com/statsmodels/statsmodels/blob/master/setup.cfg#L73

@xhochy
Copy link
Contributor Author

xhochy commented Feb 10, 2020

I'm missing something from the workflow. I've checked out this branch, added a flake-violation to groupby.pyx, but commiting it doesnt seem to cause any problem. What am I missing?

Can W293 for trailing whitespace be added?

Not a deal-breaker, but I like the more verbose way we list codes in statsmodels: https://github.com/statsmodels/statsmodels/blob/master/setup.cfg#L73

The obvious one:
Did run pre-commit install once?

Otherwise: Can you tell me what you did change? Then I can try to reproduce locally.

@xhochy
Copy link
Contributor Author

xhochy commented Feb 11, 2020

rebased and CI passed.

@WillAyd WillAyd added this to the 1.1 milestone Feb 11, 2020
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

lgtm

@jbrockmendel
Copy link
Member

Did run pre-commit install once?

Looks like brew install pre-commit followed by pre-commit install did the trick, thanks.

LGTM

W293 can be follow-up.

@WillAyd WillAyd merged commit 012a6a3 into pandas-dev:master Feb 12, 2020
@WillAyd
Copy link
Member

WillAyd commented Feb 12, 2020

Thanks @xhochy

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.

pre-commit check for flake8 doesn't check Cython code
6 participants