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

chore: expand ruff rule sets and make corresponding minor adjustments #1114

Merged
merged 32 commits into from
Feb 15, 2024

Conversation

tonyandrewmeyer
Copy link
Contributor

@tonyandrewmeyer tonyandrewmeyer commented Jan 26, 2024

This PR enables a few additional ruff checks - since they are bundled with ruff and ruff is extremely fast, they are basically free. I cherry-picked the set that I agree with/like the most - I'm happy to argue for the inclusion of any specific ones where you disagree, or consider other rules if you have preferences.

  • pyupgrade (UP) - I love this tool - I find it's a super easy way to adopt new features as you move up your minimum-supported Python version (in using it for a year or so I haven't ever had it suggest something that I didn't like)
  • flake8-2020 (YTT)
  • bandit (S) - a good tool for avoiding accidentally missing things, in my opinion (marking them with noqa shows that thought has gone into it, which has value in itself)
  • flake8-bugbear (B)
  • flake8-simplify (SIM)
  • Ruff specific (RUF) - seems like we should since we're using the tool
  • perflint (PERF) - seems to make good suggestions in general

Notes:

Where the linter picks up new issues, these are handled in one of these ways:

  • Ignore with a noqa: directive if it's a false positive or should otherwise be permanently ignored in that specific case
  • Ignore for a file or group of files (the docs and tests have several of these) where it's something we want to pick up in the core code but not everywhere
  • Ignore with a note to review later when it's likely that there would be too much additional noise in this PR
  • Make the recommended change, when it's small and seems reasonable

Continues from #1102.

Copy link
Collaborator

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Thanks! Left a few initial comments.

ops/_private/timeconv.py Outdated Show resolved Hide resolved
docs/conf.py Outdated Show resolved Hide resolved
ops/charm.py Outdated Show resolved Hide resolved
ops/framework.py Outdated Show resolved Hide resolved
ops/lib/__init__.py Outdated Show resolved Hide resolved
ops/testing.py Outdated Show resolved Hide resolved
ops/testing.py Outdated Show resolved Hide resolved
ops/testing.py Show resolved Hide resolved
test/test_charm.py Outdated Show resolved Hide resolved
test/test_infra.py Outdated Show resolved Hide resolved
docs/conf.py Outdated Show resolved Hide resolved
ops/testing.py Outdated Show resolved Hide resolved
@tonyandrewmeyer tonyandrewmeyer changed the title chore: move to ruff for linting chore: expand ruff rule sets and make corresponding minor adjustments Feb 2, 2024
@tonyandrewmeyer tonyandrewmeyer marked this pull request as ready for review February 14, 2024 06:39
@tonyandrewmeyer
Copy link
Contributor Author

One option here would be to split this into two PRs, one that has all the changes outside of pyproject.toml (and the noqa's, I guess) and then a second one that has the pyproject.toml and noqa changes, and also adds the previous commit to a .git-blame-ignore-revs file. Do you think that would be preferable? None of the code changes are functional ones, although some should be (insignificant) performance improvements.

Copy link
Collaborator

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Looks great to me, just the few things we discussed.

actions_metadata = actions_meta.read_text()
else:
actions_metadata = None
actions_metadata = actions_meta.read_text() if actions_meta.exists() else None
Copy link
Collaborator

Choose a reason for hiding this comment

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

At first I think we should disable this one, but looking over the PR I think it's almost always an improvement, so this seems reasonable to keep enabled.

ops/testing.py Outdated Show resolved Hide resolved
ops/testing.py Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
test/test_infra.py Outdated Show resolved Hide resolved
test/test_pebble.py Show resolved Hide resolved
@tonyandrewmeyer
Copy link
Contributor Author

One option here would be to split this into two PRs, one that has all the changes outside of pyproject.toml (and the noqa's, I guess) and then a second one that has the pyproject.toml and noqa changes, and also adds the previous commit to a .git-blame-ignore-revs file. Do you think that would be preferable? None of the code changes are functional ones, although some should be (insignificant) performance improvements.

We briefly discussed this and decided that it's not worth it for this PR.

@benhoyt benhoyt merged commit 1836df5 into canonical:main Feb 15, 2024
28 checks passed
@tonyandrewmeyer tonyandrewmeyer deleted the ruff-linting-1102 branch February 20, 2024 01:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants