Skip to content
This repository was archived by the owner on Jun 13, 2025. It is now read-only.

Conversation

@ajay-sentry
Copy link
Contributor

@ajay-sentry ajay-sentry commented Dec 26, 2024

Purpose/Motivation

Was looking through the ruff rules list and saw these comprehension style rules which I kinda liked. They all had unsafe-auto fixes available but I was able to go through each one and do a manual replace for all of them

If we like it we can do a similar exercise for Shared / Worker, I think the notation is a lot clearer for me but maybe folks think otherwise and we should scrap this idea.

Closes codecov/engineering-team#3151

Rule fixes were:
C401
C403
C405
C416
C417

I also added "C408" to the ignore list because there were like 400 errors for that and they were kinda annoying, particularly for logs. Maybe there were a few that were worth but overall juice wasn't worth the squeeze. Example in the screenshot below

Screenshot 2024-12-26 at 10 08 25 AM

Notes to Reviewer

Anything to note to the team? Any tips on how to review, or where to start?

Legal Boilerplate

Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.

@ajay-sentry ajay-sentry requested review from a team as code owners December 26, 2024 18:08
@codecov
Copy link

codecov bot commented Dec 26, 2024

Codecov Report

Attention: Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.

Project coverage is 96.02%. Comparing base (392c975) to head (992c39c).
Report is 4 commits behind head on main.

Changes have been made to critical files, which contain lines commonly executed in production. Learn more

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
services/bundle_analysis.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1072      +/-   ##
==========================================
- Coverage   96.02%   96.02%   -0.01%     
==========================================
  Files         828      828              
  Lines       19458    19458              
==========================================
- Hits        18685    18684       -1     
- Misses        773      774       +1     
Flag Coverage Δ
unit 92.30% <92.85%> (ø)
unit-latest-uploader 92.30% <92.85%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@github-actions
Copy link
Contributor

✅ All tests successful. No failed tests were found.

📣 Thoughts on this report? Let Codecov know! | Powered by Codecov

@codecov-notifications
Copy link

Codecov Report

Attention: Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
services/bundle_analysis.py 0.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

Copy link
Contributor

@joseph-sentry joseph-sentry left a comment

Choose a reason for hiding this comment

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

408 is probably worth adding at some point since it's: "[...] slower because the name dict must be looked up in the global scope in case it has been rebound"

@ajay-sentry ajay-sentry added this pull request to the merge queue Dec 27, 2024
Merged via the queue into main with commit db88c2d Dec 27, 2024
17 of 19 checks passed
@ajay-sentry ajay-sentry deleted the Ajay/ruff-c4-rules-test branch December 27, 2024 20:36
@Swatinem
Copy link
Contributor

Swatinem commented Jan 7, 2025

I also believe C408 should be trivially auto-fixable.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add C4 rules to API

5 participants