[`pylint`] Implement `empty-comment` (`PLR2044`) by mdbernard · Pull Request #9174 · astral-sh/ruff

mdbernard · 2023-12-18T03:49:44Z

Summary

Part of #970.

This adds Pylint's R0244 empty_comment lint as well as an always-safe fix.

Test Plan

The included snapshot verifies the following:

A line containing only a non-empty comment is not changed
A line containing leading whitespace before a non-empty comment is not changed
A line containing only an empty comment has its content deleted
A line containing only leading whitespace before an empty comment has its content deleted
A line containing only leading and trailing whitespace on an empty comment has its content deleted
A line containing trailing whitespace after a non-empty comment is not changed
A line containing only a single newline character (i.e. a blank line) is not changed
A line containing code followed by a non-empty comment is not changed
A line containing code followed by an empty comment has its content deleted after the last non-whitespace character
Lines containing code and no comments are not changed
Empty comment lines within block comments are ignored
Empty comments within triple-quoted sections are ignored

Comparison to Pylint

Running Ruff and Pylint 3.0.3 with Python 3.12.0 against the empty_comment.py file added in this PR, we see the following:

Identical behavior:
- empty_comment.py:3:0: R2044: Line with empty comment (empty-comment)
- empty_comment.py:4:0: R2044: Line with empty comment (empty-comment)
- empty_comment.py:5:0: R2044: Line with empty comment (empty-comment)
- empty_comment.py:18:0: R2044: Line with empty comment (empty-comment)
Differing behavior:
- Pylint doesn't ignore empty comments in block comments commonly used for visual spacing; I decided these were fine in this implementation since many projects use these and likely do not want them removed.
  - empty_comment.py:28:0: R2044: Line with empty comment (empty-comment)
- Pylint detects "empty comments" within the triple-quoted section at the bottom of the file, which is arguably a bug in the Pylint implementation since these are not truly comments. These are ignored by this implementation.
  - empty_comment.py:37:0: R2044: Line with empty comment (empty-comment)
  - empty_comment.py:38:0: R2044: Line with empty comment (empty-comment)
  - empty_comment.py:39:0: R2044: Line with empty comment (empty-comment)

github-actions · 2023-12-18T04:24:50Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+27 -2 violations, +0 -0 fixes in 6 projects; 35 projects unchanged)

apache/airflow (+8 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview --select ALL

+ docs/exts/docroles.py:20:1: PLR2044 [*] Line with empty comment
+ docs/exts/docroles.py:21:1: PLR2044 [*] Line with empty comment
+ tests/providers/common/sql/hooks/test_sql.py:18:1: PLR2044 [*] Line with empty comment
+ tests/providers/databricks/hooks/test_databricks_sql.py:18:1: PLR2044 [*] Line with empty comment
+ tests/providers/exasol/hooks/test_sql.py:18:1: PLR2044 [*] Line with empty comment
+ tests/providers/google/suite/transfers/test_gcs_to_gdrive.py:117:5: PLR2044 [*] Line with empty comment
+ tests/providers/snowflake/hooks/test_sql.py:18:1: PLR2044 [*] Line with empty comment
+ tests/system/providers/google/cloud/storage_transfer/example_cloud_storage_transfer_service_aws.py:121:5: PLR2044 [*] Line with empty comment

aws/aws-sam-cli (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview

+ samcli/local/docker/lambda_container.py:149:9: PLR2044 [*] Line with empty comment

milvus-io/pymilvus (+4 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview

+ pymilvus/client/abstract.py:114:9: PLR2044 [*] Line with empty comment
+ pymilvus/client/abstract.py:18:9: PLR2044 [*] Line with empty comment
+ pymilvus/client/abstract.py:99:9: PLR2044 [*] Line with empty comment
+ pymilvus/client/types.py:133:5: PLR2044 [*] Line with empty comment

pandas-dev/pandas (+2 -2 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview

+ pandas/tests/extension/decimal/array.py:175:9: PLC0415 `import` should be at the top-level of a file
- pandas/tests/extension/decimal/array.py:176:9: PLC0415 `import` should be at the top-level of a file
+ pandas/tests/extension/decimal/array.py:233:9: PLR6301 Method `_formatter` could be a function, class method, or static method
- pandas/tests/extension/decimal/array.py:234:9: PLR6301 Method `_formatter` could be a function, class method, or static method

rotki/rotki (+10 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview

+ rotkehlchen/tests/db/test_db.py:752:48: PLR2044 [*] Line with empty comment
+ rotkehlchen/tests/db/test_db.py:757:48: PLR2044 [*] Line with empty comment
+ rotkehlchen/tests/db/test_db.py:795:52: PLR2044 [*] Line with empty comment
+ rotkehlchen/tests/db/test_db.py:797:52: PLR2044 [*] Line with empty comment
+ rotkehlchen/tests/db/test_db.py:801:52: PLR2044 [*] Line with empty comment
+ rotkehlchen/tests/db/test_db.py:834:52: PLR2044 [*] Line with empty comment
+ rotkehlchen/tests/db/test_db.py:837:52: PLR2044 [*] Line with empty comment
+ rotkehlchen/tests/db/test_db.py:843:52: PLR2044 [*] Line with empty comment
+ rotkehlchen/tests/db/test_db.py:844:52: PLR2044 [*] Line with empty comment
+ rotkehlchen/tests/db/test_db.py:845:52: PLR2044 [*] Line with empty comment

zulip/zulip (+2 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview --select ALL

+ zerver/lib/export.py:842:5: PLR2044 [*] Line with empty comment
+ zerver/lib/export.py:867:5: PLR2044 [*] Line with empty comment

Changes by rule (3 rules affected)

code	total	+ violation	- violation
PLR2044	25	25	0
PLC0415	2	1	1
PLR6301	2	1	1

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

mdbernard · 2023-12-18T04:32:22Z

Looks like the ecosystem checks found some edge cases not currently covered:

"empty comment" lines within block comments, which add vertical whitespace for clarity, especially common as header comments for files
lines containing a trailing # character in multi-line comments, which should not be considered empty comments

charliermarsh · 2023-12-18T04:36:13Z

Nice, thanks for kicking this off! Agreed on the edge cases described above.

mdbernard · 2023-12-18T06:15:17Z

crates/ruff_linter/resources/test/fixtures/pylint/empty_comment.py

Add test for "empty comment" lines in multi-line strings/comments

Add test for "empty comment" lines in block comments

mdbernard · 2023-12-20T06:47:27Z

Account for comment style with trailing # at end of comment, e.g. https://github.com/rotki/rotki/blob/63302ca6d67ab928fd6cac7b6137c41df416416c/rotkehlchen/chain/ethereum/modules/l2/loopring.py#L527:

# -- Methods following the EthereumModule interface -- #

mdbernard · 2023-12-20T06:51:58Z

Account for comment style with multiple # symbols on a single line (tying in with the above comment, probably best to just lint when a comment consists of only a single # symbol followed by only whitespace through the end of the line.

mdbernard

Rebase

charliermarsh

This looks great, I really like what I'm seeing in the ecosystem checks too -- thank you!

I'm going to explore making this a token-based rule rather than a physical line-based rule, just because we want to avoid using physical lines when possible, but it'll be a mostly mechanical change...

charliermarsh · 2023-12-28T16:41:13Z

Pylint doesn't ignore empty comments in block comments commonly used for visual spacing; I decided these were fine in this implementation since many projects use these and likely do not want them removed.

I agree, we shouldn't flag these.

Pylint detects "empty comments" within the triple-quoted section at the bottom of the file, which is arguably a bug in the Pylint implementation since these are not truly comments. These are ignored by this implementation.

I agree, flagging empty comments within a multi-line string seems like a bug to me.

mdbernard · 2023-12-28T19:43:40Z

Noting that I might've missed an edge case where two empty comments exist on consecutive lines forming a "block" comment.

mdbernard · 2023-12-28T19:44:35Z

we want to avoid using physical lines when possible

Out of curiosity, why?

charliermarsh · 2023-12-29T02:14:56Z

Out of curiosity, why?

Primarily because physical line-based approaches tend to be slower, since we need to run the rule over every line. This rule is actually a good example: we already know the locations of all comments in the file (via CommentRanges), so iterating over every line, then checking if the line intersects with the comment ranges, is wasteful as compared to merely iterating over the comment ranges directly. In general, we often have information we've already computed and/or can exploit to limit the search space vs. iterating over the physical lines.

charliermarsh

This is great work! I ended up moving to the token checker, and added support for the case in which we have multiple consecutive empty comments.

mdbernard force-pushed the PLR2044-empty-comment branch 2 times, most recently from 2ef2331 to 464a094 Compare December 18, 2023 04:10

mdbernard force-pushed the PLR2044-empty-comment branch from 464a094 to f26eda6 Compare December 18, 2023 06:06

mdbernard marked this pull request as draft December 18, 2023 06:06

mdbernard commented Dec 18, 2023

View reviewed changes

mdbernard force-pushed the PLR2044-empty-comment branch from f26eda6 to c2de5d1 Compare December 20, 2023 06:27

mdbernard force-pushed the PLR2044-empty-comment branch from c2de5d1 to 3a4d1e1 Compare December 27, 2023 06:08

mdbernard commented Dec 27, 2023

View reviewed changes

mdbernard force-pushed the PLR2044-empty-comment branch 2 times, most recently from b203d24 to 09b446a Compare December 27, 2023 06:57

[pylint] add PLR2044 empty_comment

3e15b26

mdbernard force-pushed the PLR2044-empty-comment branch from 09b446a to 3e15b26 Compare December 27, 2023 07:05

mdbernard marked this pull request as ready for review December 28, 2023 02:38

charliermarsh self-requested a review December 28, 2023 14:46

Merge branch 'main' into PLR2044-empty-comment

454f8ef

charliermarsh reviewed Dec 28, 2023

View reviewed changes

Move to preview

19d9f3b

charliermarsh added 3 commits December 28, 2023 21:16

Move to token-based rule; delete entire line

72223bc

Add empty check

32119dd

Handle blank lines

20e8827

charliermarsh added the rule Implementing or modifying a lint rule label Dec 29, 2023

charliermarsh added the preview Related to preview mode features label Dec 29, 2023

charliermarsh changed the title ~~[pylint] add PLR2044 empty_comment~~ [pylint] Implement empty-comment (PLR2044) Dec 29, 2023

Fix ranges

6d78bd6

charliermarsh approved these changes Dec 29, 2023

View reviewed changes

charliermarsh enabled auto-merge (squash) December 29, 2023 02:49

charliermarsh merged commit 375c175 into astral-sh:main Dec 29, 2023

charliermarsh mentioned this pull request Jan 2, 2024

[FEATURE] Remove blank comments #4238

Closed

miccal mentioned this pull request Jan 2, 2024

ruff 0.1.10 Homebrew/homebrew-core#158827

Merged

trag1c mentioned this pull request Jan 11, 2024

Add PR title guidelines to CONTRIBUTING.md #9470

Closed

Conversation

mdbernard commented Dec 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Comparison to Pylint

Uh oh!

github-actions bot commented Dec 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Formatter (stable)

Formatter (preview)

Uh oh!

mdbernard commented Dec 18, 2023

Uh oh!

charliermarsh commented Dec 18, 2023

Uh oh!

mdbernard Dec 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdbernard commented Dec 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdbernard commented Dec 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdbernard left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charliermarsh left a comment

Choose a reason for hiding this comment

Uh oh!

charliermarsh commented Dec 28, 2023

Uh oh!

mdbernard commented Dec 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdbernard commented Dec 28, 2023

Uh oh!

charliermarsh commented Dec 29, 2023

Uh oh!

charliermarsh left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mdbernard commented Dec 18, 2023 •

edited

Loading

github-actions bot commented Dec 18, 2023 •

edited

Loading

`ruff-ecosystem` results

mdbernard Dec 18, 2023 •

edited

Loading

mdbernard commented Dec 20, 2023 •

edited

Loading

mdbernard commented Dec 20, 2023 •

edited

Loading

mdbernard left a comment •

edited

Loading

mdbernard commented Dec 28, 2023 •

edited

Loading