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: Remove unneeded occurrences of the global keyword #10232

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

techy4shri
Copy link
Contributor

@techy4shri techy4shri commented Dec 30, 2024

Part of #10196

Refactor: Removed unneeded occurrences of the global keyword (13 instances) in 7 files to improve code clarity and maintainability.

Technical

This PR removes the unneeded use of the global keyword identified by PLW0602 warnings in 7 files (13 instances).
The code has been audited to ensure functionality remains unaffected. These variables have been restructured to use local or appropriately scoped replacements.
There were quite a few depreciation errors but I left them as they were since they are not part of the issue I worked on like python depreciated methods and npm depreceated libraries.

Testing

  1. Ran ruff check . to confirm no linting errors or warnings, including PLW0602 issues.
  2. Verified all unit tests passed using pytest, confirming no regressions were introduced.
  3. Ran pre-commit run --all-files to verify things.

Screenshot

Screenshot 2024-12-28 215053

Stakeholders

@RayBB

replaced global keyword with appropriate replacements
@techy4shri
Copy link
Contributor Author

Working on the errors.

@RayBB RayBB changed the title 10196/refactor/re enable ruff rules Enable Ruff Rule: Remove unneeded occurrences of the global keyword Dec 30, 2024
@techy4shri
Copy link
Contributor Author

@RayBB can you please check whether the db_parameters error is something to be solved on my end? The issue is that dbm.ndbm doesn't have open and error attributes and the parameters and I will have to edit reveal.py file for this. Not sure if I should do that.

@RayBB
Copy link
Collaborator

RayBB commented Jan 2, 2025

@techy4shri here's the approach I'd recommend:

  1. Remove all the changes that aren't just removing the global keyword as recommended by ruff.
  2. If you're getting errors related to one of the removals for now we can add a noqa comment after that line with a note that it's needed in this instance.

For this PR we don't want to make big changes. Just address the simple cases and add an exception via noqa comments for anything that would require more work.

@techy4shri
Copy link
Contributor Author

I have only edited the files as required till now so changes needed there. It shows the same error, db_parameters are not specified in the configuration .
As for the noqa comment, I tried by adding it where the load_config() function is but it doesn't work I believe. Either I mock the config or add noqa comment in test file but then that would cause an error for others ig.

openlibrary/solr/update.py Outdated Show resolved Hide resolved
openlibrary/solr/update.py Outdated Show resolved Hide resolved
@techy4shri techy4shri requested a review from RayBB January 2, 2025 21:33
Copy link
Collaborator

@RayBB RayBB left a comment

Choose a reason for hiding this comment

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

I fixed the small issue with the data providers.
Great work on this one it is all looking good! :)

Now we just need staff to review and then merge.

@RayBB RayBB added the Needs: Staff / Internal Reviewed a PR but don't have merge powers? Use this. label Jan 3, 2025
@techy4shri
Copy link
Contributor Author

Thanks a lot! Apologies for the trouble. I'll try not to repeat this mistake next time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Affects: Developers Needs: Staff / Internal Reviewed a PR but don't have merge powers? Use this.
Projects
Status: Waiting Review/Merge from Staff
Development

Successfully merging this pull request may close these issues.

2 participants