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

Upgrade ruff=0.8.6 #5393

Merged
merged 1 commit into from
Jan 8, 2025
Merged

Upgrade ruff=0.8.6 #5393

merged 1 commit into from
Jan 8, 2025

Conversation

ogenstad
Copy link
Contributor

@ogenstad ogenstad commented Jan 7, 2025

Upgrade ruff and ignore new violations, also changes renamed rules.

Targets stable so that we can merge it back to develop as soon as this is merged.

@github-actions github-actions bot added group/backend Issue related to the backend (API Server, Git Agent) group/ci Issue related to the CI pipeline labels Jan 7, 2025
Copy link

codspeed-hq bot commented Jan 7, 2025

CodSpeed Performance Report

Merging #5393 will not alter performance

Comparing pog-upgrade-ruff (d5bbba5) with stable (a46890d)

Summary

✅ 10 untouched benchmarks

"RUF051", # [*] Use `pop` instead of `key in dict` followed by `del dict[key]`
"RUF052", # Local dummy variable `_meta` is accessed
"RUF056", # [*] Avoid providing a falsy fallback to `dict.get()` in boolean test positions. The default fallback `None` is already falsy.
"RUF100", # [*] Unused `noqa` directive (non-enabled: `INP001`)
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we just removed INP001 from the code if it's not enabled instead of adding this exception ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't want to make any code changes that weren't strictly required by this PR. For those with the [*] mark ruff should have auto-fix options and would be easy to do as follow up PRs.

"UP012", # Unnecessary call to encode as UTF-8
"UP018", # Unnecessary {literal_type} call (rewrite as a literal)
"UP031", # Use format specifiers instead of percent format
"UP035", # `typing.List` is deprecated, use `list` instead
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be good to fix UP035 and UP006 right after this PR, I hope ruff is able to automatically convert everything for us
Ideally we should also convert Optional[XX] into XX | None, this will create a lot of changes but since there aren't that many changes between stable and develop right now this is our opportunity to do it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There used to be some problems with Pydantic and using XX | None instead of Optional but I think that might have been addressed now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It worked in the SDK and I think the main issues where related to Python 3.9 so we should be good

@ogenstad ogenstad marked this pull request as ready for review January 8, 2025 06:28
@ogenstad ogenstad merged commit 57f5ea0 into stable Jan 8, 2025
35 checks passed
@ogenstad ogenstad deleted the pog-upgrade-ruff branch January 8, 2025 06:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
group/backend Issue related to the backend (API Server, Git Agent) group/ci Issue related to the CI pipeline
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants