search

boto / botocore

The low-level, core functionality of boto3 and the AWS CLI.
Apache License 2.0
1.48k stars 1.08k forks source link

Ruff update #3206

Closed nateprewitt closed 2 months ago

nateprewitt commented 3 months ago

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 commented 3 months ago

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 :warning:
botocore/docs/bcdoc/style.py 85.71% 2 Missing :warning:
botocore/paginate.py 33.33% 2 Missing :warning:
botocore/parsers.py 0.00% 2 Missing :warning:
botocore/auth.py 83.33% 1 Missing :warning:
botocore/client.py 66.66% 1 Missing :warning:
botocore/hooks.py 0.00% 1 Missing :warning:
botocore/session.py 75.00% 1 Missing :warning:
botocore/utils.py 50.00% 1 Missing :warning:
botocore/waiter.py 83.33% 1 Missing :warning:

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

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

nateprewitt commented 3 months ago

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.

SamRemis commented 3 months ago

Can we add updates to botocore/CONTRIBUTING.rst?

nateprewitt commented 3 months ago

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.