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: upgrade ruff to v0.7.0 #1437

Merged
merged 5 commits into from
Oct 23, 2024
Merged

chore: upgrade ruff to v0.7.0 #1437

merged 5 commits into from
Oct 23, 2024

Conversation

IronCore864
Copy link
Contributor

@IronCore864 IronCore864 commented Oct 18, 2024

Currently, we are using ruff 0.4.5 but recently 0.7.0 was released, this PR upgrades ruff to 0.7.0 for both formatting and linting.

Closes #1335.

@IronCore864 IronCore864 marked this pull request as ready for review October 18, 2024 02:40
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 good overall, thanks. Regarding this issue / SIM115, unless I'm missing something, it looks like it's intentionally flagging those now, so I don't think we should have TODO comments on these, right?

We should just keep the noqa: SIM115 comments as this will be always on now.

@dimaqq
Copy link
Contributor

dimaqq commented Oct 21, 2024

Looks good overall, thanks. Regarding this issue / SIM115, unless I'm missing something, it looks like it's intentionally flagging those now, so I don't think we should have TODO comments on these, right?

We should just keep the noqa: SIM115 comments as this will be always on now.

I've opened #1440 to fix this.
Fine to keep # noqa: ... or # noqa: ... # https://github.com/canonical/operator/issues/1440 until then.

Copy link
Contributor

@dimaqq dimaqq left a comment

Choose a reason for hiding this comment

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

I wonder why so few changes are required.

Perhaps it's because Tony already took care of a lot of stuff during the Scenario merge.

Or maybe it's an invitation for us re-enable some of the ignored codes?

Please check if there's some low-hanging fruit.

For example, it seems that C408 no longer needs to be ignored.

pyproject.toml Outdated Show resolved Hide resolved
@tonyandrewmeyer
Copy link
Contributor

I wonder why so few changes are required.

I suspect that using --preview helps, because that lets us be a bit ahead of rules hitting stable. We also generally write well crafted and formatted code, so there's less for the linter/formatter to find problems with. Looking over the new rules and rule changes, there's a bunch that aren't related to anything we do (e.g. async stuff).

Or maybe it's an invitation for us re-enable some of the ignored codes?

For background: most of the ones that are in pyproject.toml as ignored (in the first section) we want to keep ignoring. There are a few that have comments indicating that we'd want to consider enabling them in the future. I think mostly those will be better as separate PRs, though. If there are noqa pragmas in the code then those would be great to remove if that becomes feasible.

It would be good to check if there are any new rules/sets that we should consider enabling, either here or separately.

@IronCore864 IronCore864 requested review from dimaqq and benhoyt October 22, 2024 00:45
@IronCore864
Copy link
Contributor Author

IronCore864 commented Oct 22, 2024

Per the discussion above, we are keeping noqa for SIM115.

Per discussion in today's standup, we agreed to merge this PR first and then handle the refactor (try to remove rules that are not hitting anything or only affect a few places) in a separate PR, see it here.

@IronCore864 IronCore864 merged commit 457ca76 into main Oct 23, 2024
31 checks passed
@IronCore864 IronCore864 deleted the upgrade-ruff branch October 23, 2024 00:29
dimaqq pushed a commit that referenced this pull request Oct 24, 2024
Update linting rules with the latest ruff version 0.7.0.

After upgrading ruff to the latest version [in this
PR](#1437), some rules can be
removed since they are not hitting any code, and some other rules can be
relatively easily refactored.
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.

Upgrade ruff to latest
4 participants