Additional static analysis rules

[jtherrmann]

This issue is a place to suggest additional static analysis rules (e.g. for ruff or mypy) that we would find useful, that are not currently listed in our recommended ruff config or recommended mypy config.

I'm not sure the best way to proceed with adopting additional rules, since doing an actions release just to update the README feels a bit cumbersome. I also don't want to feel compelled to update the ruff config in all of our repos every time we add a new rule. Maybe just a bumpless merge into develop to update the README, and then incorporating the additional rules into individual repos as we see fit?

Anyway, this issue can serve as a list of proposed rule additions for now. Anyone should feel free to edit this list to add more.

Ruff

• blanket noqa comments: https://docs.astral.sh/ruff/rules/blanket-noqa/

  • not sure if this included in ruff's default rules, but we should add it if not

• unused arguments: https://docs.astral.sh/ruff/rules/#flake8-unused-arguments-arg

  • number of times encountered that was an actual mistake: 2

• mutable default arguments:

  • https://docs.astral.sh/ruff/rules/mutable-argument-default/
  • https://docs.astral.sh/ruff/rules/mutable-class-default/
  • at least a few occurrences in hyp3-isce2, e.g. swaths: list = [1, 2, 3]

Mypy

• disallow ignore comments without error code: https://mypy.readthedocs.io/en/stable/error_code_list2.html#code-ignore-without-code

Other

• redundant except statements; see Error handling for burst2safe --extent option doesn't work as intended burst2safe#129
  • feels like that would fall under "unreachable code", which ruff doesn't currently support:
    • Remove unreachable-code feature astral-sh/ruff#9463
    • Ruff should detect unreachable code but reports 'Missing explicit return', fixing it with return None astral-sh/ruff#6334
  • is there another linter that does?

[jhkennedy]

I'm not sure the best way to proceed with adopting additional rules, since doing an actions release just to update the README feels a bit cumbersome. I also don't want to feel compelled to update the ruff config in all of our repos every time we add a new rule. Maybe just a bumpless merge into develop to update the README, and then incorporating the additional rules into individual repos as we see fit?

I think if we want to adopt a new rule, this process would suffice:

1. PR to develop updating the README + Changelog stating we adopted the new rule with the pyproject.toml snippet to include
2. Release actions
3. Dependabot PRs to downstream repos can choose to include it in the pyproject.toml at that time or just add it as a commented-out rule to eventually come back to.

We don't necessarily need to release actions (step 2) if there are just README+Changelog changes, so things can sit in development ( after step 1) for a time until there's a need for an actions release, or we want to tackle the rule changes prompted b a release.

I think the challenge is making sure to come back to commented-out rules. 🤔 Could make use of a tracking issue with sub-tasks for each repo, but that is potentially likely to go out of date if everyone doesn't follow the process.

[jtherrmann]

Dependabot PRs to downstream repos can choose to include it in the pyproject.toml at that time or just add it as a commented-out rule to eventually come back to.

Yeah, I like that. Adding as a commented-out rule would increase the likelihood of remembering to enable it for repos we're actively developing.

We don't necessarily need to release actions (step 2) if there are just README+Changelog changes, so things can sit in development ( after step 1) for a time until there's a need for an actions release, or we want to tackle the rule changes prompted b a release.

We could add an [Unreleased] section to the CHANGELOG where we queue up added rules.

[jtherrmann]

I think the challenge is making sure to come back to commented-out rules. 🤔 Could make use of a tracking issue with sub-tasks for each repo, but that is potentially likely to go out of date if everyone doesn't follow the process.

I don't see us doing that en masse for all of our repos, since fixing static analysis errors in all ~23 of our Python projects at once has proved to be a very time-consuming process. But we could make a habit of uncommenting additional rules (and fixing the resulting errors) in pyproject.toml whenever we resume significant development work on a particular project.

[jhkennedy]

Looking at the hyp3lib v4 release PRs, I realized ruff doesn't enforce PEP 8 naming conventions by default. Should we consider adding the N rules(or subset thereof)?
https://docs.astral.sh/ruff/rules/#pep8-naming-n

[jtherrmann]

@jhkennedy Yeah, I think that would be ideal, if anyone has the capacity to take that on.