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

dev: Remove all the excluded ruff rules except E501 #235

Merged
merged 6 commits into from
Jun 4, 2024

Conversation

ajay-sentry
Copy link
Contributor

@ajay-sentry ajay-sentry commented May 31, 2024

Part of the lint enhancement epic: codecov/engineering-team#1716

This PR aims to fix all the "hanging" F and E rules that we excluded previously. The rules that we are now enforcing are as follows:

  • F401: {name} imported but unused; consider using importlib.util.find_spec to test for availability
  • F403: from {name} import * used; unable to detect undefined names
  • F405: {name} may be undefined, or defined from star imports
  • F841: Local variable {name} is assigned to but never used
  • E711: Comparison to None should be cond is None
  • E712: Avoid equality comparisons to True; use if {cond}: for truth checks
  • E741: Ambiguous variable name: {name}

Rules List: https://docs.astral.sh/ruff/rules/#legend

This leaves one remaining rule, E501, which is about line length unchanged. I think for now it doesn't make sense to add this as an enforced rule since it's more stylistic, but we can always consider turning it on later.

Closes codecov/engineering-team#1852

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.

Copy link

codecov bot commented May 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.54%. Comparing base (fbeae69) to head (737f316).

Current head 737f316 differs from pull request most recent head ae4af07

Please upload reports for the commit ae4af07 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #235      +/-   ##
==========================================
- Coverage   89.63%   89.54%   -0.10%     
==========================================
  Files         328      324       -4     
  Lines       10482    10306     -176     
  Branches     1913     1878      -35     
==========================================
- Hits         9396     9228     -168     
+ Misses       1023     1008      -15     
- Partials       63       70       +7     
Flag Coverage Δ
shared-docker-uploader ?

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.

@@ -26,6 +26,7 @@ exclude = [
"node_modules",
"site-packages",
"venv",
"**test**"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One thing too, since there were many, many errors in the test files was I just excluded all of them for the sake of time. If someone wants to go back and fix those as well by all means :)

@@ -16,3 +16,21 @@
ModuleReport,
)
from shared.bundle_analysis.storage import BundleAnalysisReportLoader, StoragePaths

__all__ = [
"models",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is how we can get around the "unused imports" for these init files

@@ -219,8 +219,8 @@ def diff_to_json(self, diff):
elif sol4 == "@@ -":
# ex: "@@ -31,8 +31,8 @@ blah blah blah"
# ex: "@@ -0,0 +1 @@"
l = get_start_of_line(source).groups()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"l" is an ambiguous character, and rightfully so. changed to "ln" to signal "line" which is what "l" was originally representing

@@ -802,7 +802,7 @@ async def find_pull_request(
self, commit=None, branch=None, state="open", token=None
):
state = {"open": "OPEN", "merged": "MERGED", "close": "DECLINED"}.get(state, "")
pulls, page = [], 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pulls wasn't being populated here, just an empty array

Copy link
Contributor

@JerrySentry JerrySentry left a comment

Choose a reason for hiding this comment

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

lgtm

@ajay-sentry ajay-sentry added this pull request to the merge queue Jun 4, 2024
Merged via the queue into main with commit 95c7e98 Jun 4, 2024
6 checks passed
@ajay-sentry ajay-sentry deleted the Ajay/ruff-exclude-test-remove-ignores branch June 4, 2024 21:47
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.

Add Remainder of F and E rules to Shared
2 participants