Remove unused import with ruff #423
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #423 +/- ##
==========================================
- Coverage 97.34% 97.33% -0.01%
==========================================
Files 399 399
Lines 33758 33629 -129
==========================================
- Hits 32862 32734 -128
+ Misses 896 895 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found @@ Coverage Diff @@
## main #423 +/- ##
==========================================
- Coverage 97.40% 97.39% -0.02%
==========================================
Files 398 398
Lines 33575 33444 -131
==========================================
- Hits 32703 32572 -131
Misses 872 872
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
Changes have been made to critical files, which contain lines commonly executed in production. Learn more ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #423 +/- ##
==========================================
- Coverage 97.37% 97.36% -0.01%
==========================================
Files 430 430
Lines 34449 34319 -130
==========================================
- Hits 33544 33415 -129
+ Misses 905 904 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
michelletran-codecov
force-pushed
the
remove_unused_imports
branch
3 times, most recently
from
d73c59b
to
d82ec54
Compare
|
Ooo, never herad of |
michelletran-codecov
force-pushed
the
remove_unused_imports
branch
2 times, most recently
from
d82ec54
to
6624bbd
Compare
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #423 +/- ##
=======================================
Coverage ? 97.39%
=======================================
Files ? 398
Lines ? 33444
Branches ? 0
=======================================
Hits ? 32572
Misses ? 872
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
I've tested this in staging and was able go through the following workflow:
Are there any other workflows that would be useful to check? I will probably do a quick check before merging again. |
|
@michelletran-codecov those are good flows! I think by virtue of the change living in staging for some time (a day or two?) we can notice any error trickle in. I'd just check sentry every now and then and ensure nothing is off. But should be good! |
| import pytest | ||
|
|
||
| import database.events | ||
| import database.events # noqa: F401 |
There was a problem hiding this comment.
Soo, I searched this up and this F401 is an unused imported module, https://www.flake8rules.com/rules/F401.html#:~:text=A%20module%20has%20been%20imported,the%20import%20should%20be%20removed. Do we want to get rid of this line as part of running ruff? Or do we just want to signal it?
(I think we should remove it, maybe it was missed?)
There was a problem hiding this comment.
Ah, this was actually removed, and the test started failing. :p So I added it back. Admittedly, I'm not entirely sure why it's required.
There was a problem hiding this comment.
Interesting.. that's werid haha, I guess ruff can tell? Maybe there's methods from database.events that are silently used, but ya that's odd
| @@ -1,3 +1,4 @@ | |||
| # ruff: noqa: F401 | |||
There was a problem hiding this comment.
Same here about F401
| @@ -1,3 +1,4 @@ | |||
| # ruff: noqa: F401 | |||
There was a problem hiding this comment.
Same here, but now wondering if this should be treated differently for init.py for imports, so this one seems okay?
There was a problem hiding this comment.
Ah, I was trying to be conservative (especially with these imports in __init__). I'll try removing some of these and see how it goes.🤞
There was a problem hiding this comment.
Hmm... these paths are being imported elsewhere in the code. I would have to update the downstream references if we want to get rid of them here. Since this provides a shorter import path for code importing these modules, so I don't think there's any harm leaving them as is.
In particular, I was interested in F401. This should clean up all unused imports in the repo. We have to be careful with celery tasks because they don't get picked up if they are not "imported" anywhere. I've added an annotation on the `__init__` files that import these "unused" tasks.
michelletran-codecov
force-pushed
the
remove_unused_imports
branch
from
6624bbd
to
ead75d6
Compare
michelletran-codecov
temporarily deployed
to
staging
GitHub Actions
Inactive
In particular, I was interested in F401. This should clean up all unused imports in the repo. We have to be careful with celery tasks because they don't get picked up if they are not "imported" anywhere. I've added an annotation on the
__init__files that import these "unused" tasks.I've also switched to using ruff for "black" linting, which behaves a bit differently.
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.