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

Ruff update #3206

Merged
merged 4 commits into from
Jul 22, 2024
Merged

Ruff update #3206

merged 4 commits into from
Jul 22, 2024

Conversation

nateprewitt
Copy link
Contributor

This is a follow up to boto/boto3#4161 to move our repos to use ruff in favor of our existing pre-commit cocktail. For botocore, we have some unused imports that have been left in place due to third-party packages importing from modules importing from other modules. ruff does not currently supply a way to comprehensively skip specific lines, only disabling sorting for entire import blocks. Because of this we've shut off sorting in ruff and will keep isort in place for now.

This should be largely inline with our existing configuration with some minor changes to fix f-string upgrades missed with our previous setup and some newline fixes. This PR keeps our existing quote preservation behavior until we get all projects moved to ruff. At that point, we can look at turning it to "double".

@codecov-commenter
Copy link

codecov-commenter commented Jun 14, 2024

Codecov Report

Attention: Patch coverage is 82.97872% with 16 lines in your changes missing coverage. Please review.

Project coverage is 93.15%. Comparing base (3faa7f5) to head (cf10433).
Report is 32 commits behind head on develop.

Files Patch % Lines
botocore/configprovider.py 0.00% 4 Missing ⚠️
botocore/docs/bcdoc/style.py 85.71% 2 Missing ⚠️
botocore/paginate.py 33.33% 2 Missing ⚠️
botocore/parsers.py 0.00% 2 Missing ⚠️
botocore/auth.py 83.33% 1 Missing ⚠️
botocore/client.py 66.66% 1 Missing ⚠️
botocore/hooks.py 0.00% 1 Missing ⚠️
botocore/session.py 75.00% 1 Missing ⚠️
botocore/utils.py 50.00% 1 Missing ⚠️
botocore/waiter.py 83.33% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #3206   +/-   ##
========================================
  Coverage    93.14%   93.15%           
========================================
  Files           66       66           
  Lines        14248    14249    +1     
========================================
+ Hits         13272    13273    +1     
  Misses         976      976           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@jonathan343 jonathan343 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, just had a few questions:

  • We should add a .git-blame-ignore-revs entry for the related commits like we did for boto3: Add .git-blame-ignore-revs boto3#4168
  • I think we're making logging slightly inefficient by migrating onto fstrings. I'm not strongly against this since performance hit should be insignificant, but wanted to make sure this is a conscious change.
  • Is there a reason for keeping isort?

@nateprewitt
Copy link
Contributor Author

We should add a .git-blame-ignore-revs entry for the related commits like we did for boto3

I usually save these for follow up PRs since we have to change the commit every time we force push to this branch and if this gets squash merged we have to redo the hash as well. Those have been messed up enough that just keeping it as a post-release step has been most effective.

I think we're making logging slightly inefficient by migrating onto fstrings. I'm not strongly against this since performance hit should be insignificant, but wanted to make sure this is a conscious change.

Hmm, good shout. So this is interesting, Jonas pointed out a minor perf improvement for using logger.debug("message %s", value) over the f-strings which is why we've gone that direction in net-new code. Almost everything this is modifying is turning code in the format logger.debug("message %s" % value) into f-strings which is more performant (note % vs ,). There are two outliers though:

I don't actually know why these got reformatted, but lines like this didn't:

I can take a look at trying to manually move the first two lines back to the older format. Rewriting everything to the comma format by hand is likely not worth the minor performance difference imo.

Is there a reason for keeping isort?

I added a blip about that in the overview of the PR. The main issue is ruff does not have a way to ignore specific import lines only blocks. We have to keep isort for backwards compatibility at the moment.

Copy link
Contributor

@jonathan343 jonathan343 left a comment

Choose a reason for hiding this comment

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

Cool, thanks for the insight. I don't think that issue is worth blocking this PR. Approving

@SamRemis
Copy link
Contributor

Can we add updates to botocore/CONTRIBUTING.rst?

@nateprewitt
Copy link
Contributor Author

Yep, there are still a number of cleanup actions needed. The PR is still in draft to be added to as we go. I'll get everything finalized before putting it up for review.

@nateprewitt nateprewitt marked this pull request as ready for review July 22, 2024 20:41
Copy link
Contributor

@jonathan343 jonathan343 left a comment

Choose a reason for hiding this comment

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

🚀

@nateprewitt nateprewitt merged commit fe6e5c5 into boto:develop Jul 22, 2024
34 checks passed
@nateprewitt nateprewitt deleted the ruff_update branch July 22, 2024 21:02
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.

4 participants