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

enable ruff rule to check for unused imports #9182

Merged
merged 8 commits into from
May 22, 2024

Conversation

RayBB
Copy link
Collaborator

@RayBB RayBB commented Apr 29, 2024

Closes #9181

Technical

Testing

The tests are passing, and things work fine locally. But we should be thoughtful about if any of these removed imports have side effects we need.

Screenshot

Stakeholders

@RayBB RayBB marked this pull request as draft April 29, 2024 01:36
@RayBB RayBB marked this pull request as ready for review April 29, 2024 02:09
Copy link
Collaborator

@scottbarnes scottbarnes left a comment

Choose a reason for hiding this comment

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

I've been testing this one locally, and on the one hand it appears to be working, but on the other hand is scares me a lot because of the way some of these imports give the appearance of being unused.

I am putting this on testing to see how it goes. Thanks for all your work on this so far, @RayBB. I definitely want to see this merged.

@scottbarnes scottbarnes added the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label May 6, 2024
@RayBB
Copy link
Collaborator Author

RayBB commented May 6, 2024

@scottbarnes I agree it is a bit tricky to feel confident in this. I tried to be a bit conservative in keeping many that were kinda odd but would be good to review with someone who knows these imports better.
We could also do this a bit more piecemeal by doing a few smaller PRs to remove the ones we have high confidence in but it would be quite a bit more work.

@tfmorris
Copy link
Contributor

tfmorris commented May 6, 2024

It's probably OK to remove unused imports for standard library modules, but anything in the openlibrary/infogami/web.py sphere I'd be exceedingly careful of. The have been multiple attempts to do this and people always end up putting them back when they discover hidden side effect dependencies.

@mekarpeles mekarpeles added the Needs: Special Deploy This PR will need a non-standard deploy to production label May 13, 2024
@mekarpeles mekarpeles added the State: Blocked Work has stopped, waiting for something (Info, Dependent fix, etc. See comments). [managed] label May 20, 2024
@mekarpeles
Copy link
Member

Seems like it's in a good state, but we may not know what we don't know, so let's talk once more during 1:1 on Wed @scottbarnes and make sure we're happy with the approach (or whether we want to roll this out in stages)

@mekarpeles
Copy link
Member

I've done a review, my hesitation would be that there could be prod instances (like crons on other tasks that require these imports in other contexts) however, I think TIAS (try it and see) seems like a reasonable approach given having reviewed the specific contexts of these changes and my perceived risks of them. Especially since it's marked as Needs: Special Deploy This PR will need a non-standard deploy to production let's merge, monitor, and be ready to roll back.

@mekarpeles
Copy link
Member

@scottbarnes ready to go, my apologies though if you're willing to tweak this pr against the follow changes, we should be good to merge.

@scottbarnes scottbarnes force-pushed the fix/ruff-check-unused-import branch from aa29920 to b951159 Compare May 22, 2024 20:13
@scottbarnes scottbarnes merged commit b72499c into master May 22, 2024
4 checks passed
@scottbarnes scottbarnes deleted the fix/ruff-check-unused-import branch May 22, 2024 20:15
@RayBB
Copy link
Collaborator Author

RayBB commented May 22, 2024

@mekarpeles @scottbarnes excellent. I think this will be a big win for keeping the code clean. Fingers crossed to not hit any snags with the deploy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Special Deploy This PR will need a non-standard deploy to production On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing State: Blocked Work has stopped, waiting for something (Info, Dependent fix, etc. See comments). [managed]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

enable ruff rule: unused-import (F401)
4 participants