mistercrunch commented on PR #31447:
URL: https://github.com/apache/superset/pull/31447#issuecomment-2547356366
Just to clarify - this PR enforces new rules, that either were not found by
`pylint` before (now deprecated in this repo) or weren't applied to certain
folders.
These new rules are for the most part virtuous, and will apply to all **new
code** getting committed from this point forward.
For older code, I didn't want to go and potentially break things. The goal
of this PR is to force these linting rules moving forward.
Going and linting older code is great, and we welcome people to go and fix
the rules where they apply. In some cases we don't want to apply these rules
for various/specific reasons. Not because they're not great, but because they
don't apply to certain areas of the code base. I'll break down some examples
later in this comment.
Here's some stats about all the lines marked `--noqa` (note that it's easy
to `--ignore-noqa` for cases where we want to go and apply new rules to older
code that now has `# noqa`).
```
$ ruff check --ignore-noqa --statistics | head -n 25
709 E501 [ ] line-too-long
671 E402 [ ] module-import-not-at-top-of-file
121 F405 [ ] undefined-local-with-import-star-usage
84 C901 [ ] complex-structure
84 N806 [ ] non-lowercase-variable-in-function
79 PT027 [ ] pytest-unittest-raises-assertion
59 N813 [ ] camelcase-imported-as-lowercase
57 N815 [ ] mixed-case-variable-in-class-scope
50 S311 [ ] suspicious-non-cryptographic-random-usage
42 S105 [ ] hardcoded-password-string
39 F401 [ ] unused-import
38 C408 [*] unnecessary-collection-call
35 S110 [ ] try-except-pass
33 S608 [ ] hardcoded-sql-expression
28 S106 [ ] hardcoded-password-func-arg
26 PT009 [ ] pytest-unittest-assertion
20 PT011 [ ] pytest-raises-too-broad
20 PT012 [ ] pytest-raises-with-multiple-statements
13 B007 [ ] unused-loop-control-variable
13 C400 [*] unnecessary-generator-list
13 C416 [*] unnecessary-comprehension
13 N802 [ ] invalid-function-name
12 N818 [ ] error-suffix-on-exception-name
11 N803 [ ] invalid-argument-name
11 F403 [ ] undefined-local-with-import-star
```
## Examples of valid NOQA
### line too long
in this case, we have:
- some suffixed comments that overflows the line length, more elegant maybe
than putting the noqa instruction before
- some docstings with data tables
- things in unit tests
- ...
```
$ ruff check --ignore-noqa --select E501
tests/unit_tests/jinja_context_test.py:1001:89: E501 Line too long (97 > 88)
|
999 | TimeFilter(
1000 | from_expr="TO_TIMESTAMP('2024-08-27 00:00:00.000000',
'YYYY-MM-DD HH24:MI:SS.US')", # noqa: E501
1001 | to_expr="TO_TIMESTAMP('2024-09-03 00:00:00.000000',
'YYYY-MM-DD HH24:MI:SS.US')", # noqa: E501
|
^^^^^^^^^ E501
1002 | time_range="Last week",
1003 | ),
|
tests/unit_tests/pandas_postprocessing/test_compare.py:165:89: E501 Line too
long (121 > 88)
|
163 | flat_df = pp.flatten(post_df)
164 | """
165 | __timestamp difference__m1__m2, a, x difference__m1__m2, a, y
difference__m1__m2, b, x difference__m1__m2, b, y
|
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ E501
166 | 0 2021-01-01 0 0
0 0
167 | 1 2021-01-02 0 0
0 0
```
### module-import-not-at-top-of-file
in case of module-import, auto-generated migrations files from `alembic`
break that rule.
```
18:11 $ ruff check --ignore-noqa --select E402 | head -n 30
superset/migrations/versions/2015-09-21_17-30_4e6a06bad7a8_init.py:29:1:
E402 Module level import not at top of file
|
27 | down_revision = None
28 |
29 | import sqlalchemy as sa # noqa: E402
| ^^^^^^^^^^^^^^^^^^^^^^^ E402
30 | from alembic import op # noqa: E402
|
superset/migrations/versions/2015-09-21_17-30_4e6a06bad7a8_init.py:30:1:
E402 Module level import not at top of file
|
29 | import sqlalchemy as sa # noqa: E402
30 | from alembic import op # noqa: E402
| ^^^^^^^^^^^^^^^^^^^^^^ E402
|
superset/migrations/versions/2015-10-05_10-32_5a7bad26f2a7_.py:29:1: E402
Module level import not at top of file
|
27 | down_revision = "4e6a06bad7a8"
28 |
29 | import sqlalchemy as sa # noqa: E402
| ^^^^^^^^^^^^^^^^^^^^^^^ E402
30 | from alembic import op # noqa: E402
|
superset/migrations/versions/2015-10-05_10-32_5a7bad26f2a7_.py:30:1: E402
Module level import not at top of file
|
29 | import sqlalchemy as sa # noqa: E402
30 | from alembic import op # noqa: E402
| ^^^^^^^^^^^^^^^^^^^^^^ E402
```
So yes, in many cases it's fairly reasonable to mark things as noqa. And we
do want to enforce these rules moving forward, maybe not all-of-them
all-the-time, but they're good to have.
Are there things we should fix that aren't fixed in this PR? Totally. But
fixing them involves risk in breaking things, like camel-case in the backend,
sure we could fix them, but may break frontend/backend communications.
I say we enforce more rules moving forward, and flag as noqa where it
doesn't apply.
One more thing - I didn't run the `--unsafe-fixes` option that `ruff`
exposes. I'm guessing it's pretty safe, but needs review/test-coverage to push
through.
In any case. If I have to go and fix all the instances of issues that exist
in order to enforce these rules, I'll probably bail. Now with ruff we have the
luxury to apply new rules moving forward, and have good tooling that can
`--add-noqa` easily, and `--ignore-noqa` for when we want to go and refactor.
Thinking this PR is great net positive to ensure lint rules **moving forward**.
As I said before, we can fine tune which rules we like or don't like. I
think this is what the conversation should be about. Now that we have a blazing
fast, centralized linter it's easy to switch things on and off.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
