Skip to content
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

TD003: remove issue code length restriction #15175

Merged

Conversation

mdbernard
Copy link
Contributor

Summary

My company has issue codes longer than 6 characters, and I imagine we're not the only ones. The current maximum value of 6 characters seems arbitrary (please correct me if I'm missing something), so I chose the value of 12, since that would easily cover every issue code my company uses (and I sincerely doubt there are very many users using codes longer than this).

Test Plan

I added some additional snapshot checks which cover the new boundary conditions.

Copy link
Contributor

github-actions bot commented Dec 29, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@mdbernard mdbernard marked this pull request as draft December 29, 2024 01:13
@mdbernard mdbernard force-pushed the TD003-increase-max-issue-code-length branch from 3b0a1be to 0991625 Compare December 29, 2024 01:14
@mdbernard mdbernard marked this pull request as ready for review December 29, 2024 01:20
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Nice, thank you.

Our current implementation matches the Regex of the upstream flake8-todos implementation. I tried to figure out the reason from the upstream blame but without success, because the blame doesn't allow me to go past the most recent revision for the regex line). I also checked the PR adding rule to see if there's any discussion around what issue formats to support, but I couldn't find any.

That said, I think it's fine to remove the length restriction. It helps to remove some false positives, like in your case, and I think it's unlikely to reduce true positives, considering that the format is unlikely to be used in a regular comment. I'd prefer to remove it entirely instead of just bumping it to 12.

We also need to update the rule's documentation because it mentions up to 6 which would now be outdated.

crates/ruff_linter/src/rules/flake8_todos/rules/todos.rs Outdated Show resolved Hide resolved
@MichaReiser MichaReiser added the rule Implementing or modifying a lint rule label Dec 30, 2024
@MichaReiser MichaReiser changed the title TD003: increase max issue code length to 12 characters TD003: remove issue code length restriction Jan 3, 2025
@MichaReiser MichaReiser enabled auto-merge (squash) January 3, 2025 09:35
@MichaReiser MichaReiser disabled auto-merge January 3, 2025 09:38
@MichaReiser MichaReiser merged commit 0dbfa8d into astral-sh:main Jan 3, 2025
21 checks passed
dcreager added a commit that referenced this pull request Jan 3, 2025
* main:
  [`ruff`] Avoid reporting when `ndigits` is possibly negative (`RUF057`) (#15234)
  Attribute panics to the mdtests that cause them (#15241)
  Show errors for attempted fixes only when passed `--verbose` (#15237)
  [`RUF`] Add rule to detect empty literal in deque call (`RUF025`) (#15104)
  TD003: remove issue code length restriction (#15175)
  Preserve multiline implicit concatenated strings in docstring positions (#15126)
  [`pyflakes`] Ignore errors in `@no_type_check` string annotations (`F722`, `F821`) (#15215)
  style(AIR302): rename removed_airflow_plugin_extension as check_airflow_plugin_extension (#15233)
  [`pylint`] Re-implement `unreachable` (`PLW0101`) (#10891)
  refactor(AIR303): move duplicate qualified_name.to_string() to Diagnostic argument (#15220)
  Misc. clean up to rounding rules (#15231)
  Avoid syntax error when removing int over multiple lines (#15230)
  Migrate renovate config (#15228)
  Remove `Type::tuple` in favor of `TupleType::from_elements` (#15218)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants