[flake8-todo] - Align TD003 with the VSCode Github PR extension
#9627
Conversation
MartinBernstorff
changed the title
TD003 with the VSCode Github PR extensionflake8-todo] Align TD003 with the VSCode Github PR extension
MartinBernstorff
changed the title
flake8-todo] Align TD003 with the VSCode Github PR extensionflake8-todo] - Align TD003 with the VSCode Github PR extension
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| TD003 | 1376 | 678 | 698 | 0 | 0 |
Linter (preview)
ℹ️ ecosystem check detected linter changes. (+678 -698 violations, +0 -0 fixes in 3 projects; 40 projects unchanged)
apache/airflow (+275 -276 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --preview --select ALL
+ airflow/api_connexion/schemas/dag_schema.py:100:55: TD003 Missing issue link for this TODO - airflow/api_connexion/schemas/dag_schema.py:100:55: TD003 Missing issue link on the line following this TODO + airflow/cli/cli_config.py:239:6: TD003 Missing issue link for this TODO - airflow/cli/cli_config.py:239:6: TD003 Missing issue link on the line following this TODO + airflow/cli/cli_config.py:579:3: TD003 Missing issue link for this TODO - airflow/cli/cli_config.py:579:3: TD003 Missing issue link on the line following this TODO + airflow/cli/cli_config.py:595:3: TD003 Missing issue link for this TODO - airflow/cli/cli_config.py:595:3: TD003 Missing issue link on the line following this TODO + airflow/cli/commands/task_command.py:191:11: TD003 Missing issue link for this TODO - airflow/cli/commands/task_command.py:191:11: TD003 Missing issue link on the line following this TODO + airflow/cli/commands/task_command.py:237:15: TD003 Missing issue link for this TODO - airflow/cli/commands/task_command.py:237:15: TD003 Missing issue link on the line following this TODO + airflow/cli/commands/task_command.py:472:7: TD003 Missing issue link for this TODO - airflow/cli/commands/task_command.py:472:7: TD003 Missing issue link on the line following this TODO + airflow/cli/commands/task_command.py:689:11: TD003 Missing issue link for this TODO - airflow/cli/commands/task_command.py:689:11: TD003 Missing issue link on the line following this TODO + airflow/compat/functools.pyi:20:3: TD003 Missing issue link for this TODO - airflow/compat/functools.pyi:20:3: TD003 Missing issue link on the line following this TODO + airflow/dag_processing/manager.py:1293:7: TD003 Missing issue link for this TODO - airflow/dag_processing/manager.py:1293:7: TD003 Missing issue link on the line following this TODO ... 531 additional changes omitted for project
bokeh/bokeh (+180 -198 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --preview --select ALL
+ examples/integration/layout/plot_fixed_frame_size.py:38:3: TD003 Missing issue link for this TODO - examples/integration/layout/plot_fixed_frame_size.py:38:3: TD003 Missing issue link on the line following this TODO + examples/models/legends.py:53:11: TD003 Missing issue link for this TODO - examples/models/legends.py:53:11: TD003 Missing issue link on the line following this TODO + examples/server/app/simple_hdf5/main.py:50:3: TD003 Missing issue link for this TODO - examples/server/app/simple_hdf5/main.py:50:3: TD003 Missing issue link on the line following this TODO + release/credentials.py:77:7: TD003 Missing issue link for this TODO - release/credentials.py:77:7: TD003 Missing issue link on the line following this TODO + release/credentials.py:84:7: TD003 Missing issue link for this TODO - release/credentials.py:84:7: TD003 Missing issue link on the line following this TODO + src/bokeh/application/application.py:187:15: TD003 Missing issue link for this TODO - src/bokeh/application/application.py:187:15: TD003 Missing issue link on the line following this TODO + src/bokeh/application/handlers/code.py:168:11: TD003 Missing issue link for this TODO ... 365 additional changes omitted for project
zulip/zulip (+223 -224 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --preview --select ALL
+ analytics/lib/counts.py:122:7: TD003 Missing issue link for this TODO - analytics/lib/counts.py:122:7: TD003 Missing issue link on the line following this TODO + analytics/lib/counts.py:266:11: TD003 Missing issue link for this TODO - analytics/lib/counts.py:266:11: TD003 Missing issue link on the line following this TODO + analytics/management/commands/populate_analytics_db.py:74:11: TD003 Missing issue link for this TODO - analytics/management/commands/populate_analytics_db.py:74:11: TD003 Missing issue link on the line following this TODO + analytics/views/stats.py:111:11: TD003 Missing issue link for this TODO - analytics/views/stats.py:111:11: TD003 Missing issue link on the line following this TODO + analytics/views/stats.py:451:11: TD003 Missing issue link for this TODO - analytics/views/stats.py:451:11: TD003 Missing issue link on the line following this TODO + corporate/lib/stripe.py:1343:11: TD003 Missing issue link for this TODO - corporate/lib/stripe.py:1343:11: TD003 Missing issue link on the line following this TODO + corporate/lib/stripe.py:1923:11: TD003 Missing issue link for this TODO - corporate/lib/stripe.py:1923:11: TD003 Missing issue link on the line following this TODO + corporate/lib/stripe.py:2190:15: TD003 Missing issue link for this TODO - corporate/lib/stripe.py:2190:15: TD003 Missing issue link on the line following this TODO ... 431 additional changes omitted for project
Changes by rule (1 rules affected)
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| TD003 | 1376 | 678 | 698 | 0 | 0 |
There was a problem hiding this comment.
Thanks for putting together this PR!
It seems that there are lot of violations which got removed from the ecosystem checks. For example, https://github.com/apache/airflow/blob/c0f76013917ee57b3cc2cebcf08e4421143eefc7/airflow/dag_processing/manager.py#L1293 isn't being triggered now. I think the issue might be the re-ordering of the code as there's an issue link after a couple of lines of the TODO in the source code.
| r"^#\s*(http|https)://.*", // issue link | ||
| r"^#\s*\d+$", // issue code - like "003" | ||
| r"^#\s*[A-Z]{1,6}\-?\d+$", // issue code - like "TD003" | ||
| r"^#.*(http|https)://.*", // issue link | ||
| r"^#.*#\d+", // issue code - like "#003" | ||
| r"^#\s\d+", // issue code - like "003" | ||
| r"^#\s[A-Z]{1,6}\-?\d+", // issue code - like "TD003" |
There was a problem hiding this comment.
Is it correct that the reason you removed the \s from the "issue link" regex is so that the link can be anywhere in the comment?
Also, can we keep the asterisk for \s* so that there can be any number of whitespace instead of just one? Is there a reason you removed them?
There was a problem hiding this comment.
Is it correct that the reason you removed the \s from the "issue link" regex is so that the link can be anywhere in the comment?
Yep, absolutely!
Also, can we keep the asterisk for \s* so that there can be any number of whitespace instead of just one? Is there a reason you removed them?
I removed 'em because I'd consider # TODO incorrect, but if we were to enforce that, it should be through TD007. I've re-added them.
| 32 | | ||
| 33 | # TODO: https://github.com/astral-sh/ruff/issues/8061 This comment also has a link |
There was a problem hiding this comment.
Can you add a test case for #002 variant as well?
There was a problem hiding this comment.
Yep! It's at the bottom of TD003.py, but doesn't error, so is not in the .snap. The link-case is included due to being within the context of the non-linnk failing example :-)
This is a really good point! I'm implementing a solution that should make the changes smaller in the ecosystem. That said, I think this opens up a discussion about desirable behaviour. E.g. the linked comment does, in fact, contain an issue link. It just doesn't occur on the same or the next line. Would we want to support that, or is the false-negative rate perhaps too high? I'd lean towards "on same or next line", but would love to hear your thoughts! |
CodSpeed Performance ReportMerging #9627 will not alter performanceComparing Summary
|
|
The large amount of ecosystem changes in the previous version was largely due to changing the regex set. I've had to split into two regex sets to keep the ecosystem changes small. Unsure if that should give such a large change to performance, would love some input here. Let me know what you think! |
Yeah, I'm thinking in the same direction. It seems like the extension only provides the code action to create the issue if the text / link isn't on the same line. So, your intuition to making it work for same or next line should reduce the false negative rate. |
|
This seems to have gotten a bit stale, but it would still be very nice to get this. Can I assist in any way to help push this forward? |
|
Hi @simonpercivall! Absolutely, it's on me for not finishing this. I'm not sure I ever will, so from my end you're free to fork/replace this PR 👍 |
|
(I've merged the latest |
|
Gonna move this work to #15519 so that we can work off of main. (I tried rebasing first but was not confident in some of the messier conflict resolutions, so it seemed cleaner to do it this way). @MartinBernstorff thank you for the contribution, and sorry for the long delay in resolution! |
…sing-todo-link` (`TD003`) (#15519) ## Summary Allow links to issues that appear on the same line as the TODO directive, if they conform to the format that VSCode's GitHub PR extension produces. Revival of #9627 (the branch was stale enough that rebasing was a lot harder than just making the changes anew). Credit should go to the author of that PR though. Closes #8061 Co-authored-by: Martin Bernstorff <martinbernstorff@gmail.com>
|
@dylwil3 Absolutely no worries, I didn't follow up either, and super happy to see this merged! |
Summary
The GitHub VSCode extension supports automatically creating an issue from a comment with two formats, selectable under the setting
githubIssues.createInsertFormat.As discussed in #8061, this PR expands
TD003to consider those two formats valid.Design considerations
Since the new behaviour is single-line analysis, I considered adding it to the
static_errorsfn, but decided co-locating all theTD003code instead made more sense.Test Plan
The two formats were added to
TD003.pyand did not raise errors. I'm somewhat concerned that the new regex patterns could induce false negatives, and would love reviewer feedback on that!