-
-
Notifications
You must be signed in to change notification settings - Fork 25.5k
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
MAINT Automatic Cython linting #25861
Changes from all commits
15ba44c
95266a6
9becad8
273d513
d002019
ac87cba
7293cd4
ba3979f
920df1b
c59f028
b03e25e
da90347
182b965
d14ed54
213b991
2afba48
2f88426
e25e24d
82e4ea3
114960f
7a63d58
6f5e4cc
cb7d18e
b2fe3f7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,3 +38,49 @@ exclude = ''' | |
| asv_benchmarks/env | ||
)/ | ||
''' | ||
|
||
[tool.cython-lint] | ||
# Ignore the same error codes as flake8 | ||
# + E501 (line too long) because keeping it < 88 in cython | ||
# often makes code less readable. | ||
ignore = [ | ||
# check ignored by default in flake8. Meaning unclear. | ||
'E24', | ||
# space before : (needed for how black formats slicing) | ||
'E203', | ||
# line too long | ||
'E501', | ||
# do not assign a lambda expression, use a def | ||
'E731', | ||
# do not use variables named 'l', 'O', or 'I' | ||
'E741', | ||
# line break before binary operator | ||
'W503', | ||
# line break after binary operator | ||
'W504', | ||
] | ||
# Exclude files are generated from tempita templates | ||
exclude= ''' | ||
( | ||
sklearn/_loss/_loss.pyx | ||
| sklearn/linear_model/_sag_fast.pyx | ||
| sklearn/linear_model/_sgd_fast.pyx | ||
| sklearn/utils/_seq_dataset.pyx | ||
| sklearn/utils/_seq_dataset.pxd | ||
| sklearn/utils/_weight_vector.pyx | ||
| sklearn/utils/_weight_vector.pxd | ||
| sklearn/metrics/_dist_metrics.pyx | ||
| sklearn/metrics/_dist_metrics.pxd | ||
| sklearn/metrics/_pairwise_distances_reduction/_argkmin.pxd | ||
| sklearn/metrics/_pairwise_distances_reduction/_argkmin.pyx | ||
| sklearn/metrics/_pairwise_distances_reduction/_argkmin_classmode.pyx | ||
| sklearn/metrics/_pairwise_distances_reduction/_base.pxd | ||
| sklearn/metrics/_pairwise_distances_reduction/_base.pyx | ||
| sklearn/metrics/_pairwise_distances_reduction/_datasets_pair.pxd | ||
| sklearn/metrics/_pairwise_distances_reduction/_datasets_pair.pyx | ||
| sklearn/metrics/_pairwise_distances_reduction/_middle_term_computer.pxd | ||
| sklearn/metrics/_pairwise_distances_reduction/_middle_term_computer.pyx | ||
| sklearn/metrics/_pairwise_distances_reduction/_radius_neighbors.pxd | ||
| sklearn/metrics/_pairwise_distances_reduction/_radius_neighbors.pyx | ||
) | ||
''' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about using I am fine with both approaches, but maybe the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We now have three lists to maintain (this one, the one in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
otherwise, I guess you could make a local script files = subprocess.run(['git ls-files'], text=True, capture_output=True).strip().splitlines()
subprocess.run(['cython-lint', *files]) and then both contributors and CI run Or write another script to keep the three lists in sync and have it run in CI? This might be smallest-effort, least disruptive solution? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't want to force our contributors to use
I like the idea of contributors being able to invoke the cython linter the same way they invoke the other linters. We have a list of excluded directories/files for black in |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,18 +33,8 @@ target-version = ['py37'] | |
ignore= | ||
# check ignored by default in flake8. Meaning unclear. | ||
E24, | ||
# continuation line under-indented | ||
E121, | ||
# closing bracket does not match indentation | ||
E123, | ||
# continuation line over-indented for hanging indent | ||
E126, | ||
# space before : (needed for how black formats slicing) | ||
E203, | ||
# missing whitespace around arithmetic operator | ||
E226, | ||
# multiple statements on one line (def) | ||
E704, | ||
Comment on lines
-36
to
-47
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All these error codes seem outdated since we are black compliant. I check removing them and we indeed don't violate any of them. I think it's a good opportunity to do some clean-up |
||
# do not assign a lambda expression, use a def | ||
E731, | ||
# do not use variables named 'l', 'O', or 'I' | ||
|
@@ -82,6 +72,7 @@ allow_redefinition = True | |
ignore = | ||
sklearn/_loss/_loss.pyx | ||
sklearn/linear_model/_sag_fast.pyx | ||
sklearn/linear_model/_sgd_fast.pyx | ||
sklearn/utils/_seq_dataset.pyx | ||
sklearn/utils/_seq_dataset.pxd | ||
sklearn/utils/_weight_vector.pyx | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally those dependencies should all be locked and maintained via our script in
build_tools
but we can do that later.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but this is not run within the same environment