Re-enable several disabled ruff rules

[RayBB]

Proposal

Today I went through all the ruff rules we manually disabled here and found several rules that likely should be re-enabled to improve the codebase.

To understand any of the above rules better search for them on https://docs.astral.sh/ruff/

Want to contribute to this epic? 🏃 Let's improve our code quality together!

Step-by-Step Guide for Contributors

1. Pick a Rule to Work On

   • Look through the rules listed below
   • Choose one that interests you and matches your skill level
   • Make sure no one else has claimed it (no name next to it)
   • Make sure no one has commented asking to work on it

2. Understand the Rule

   • Visit https://docs.astral.sh/ruff/
   • Search for your chosen rule
   • Read the documentation carefully to understand what the rule checks for
   • Look at the examples of good and bad code

3. Claim Your Rule

   • Comment on this issue with: "I'd like to work on rule X"
   • I'll add your name/date next to the rule soon but you can start looking at it now
   • Note: You won't be formally assigned to this issue since many people will be working on it
   • Only work on one rule at a time. When your PR is merged you can select another.

4. Make the Changes

   • Remove your chosen rule from the ignore list in pyproject.toml
   • Run pre-commit run --all-files to see what needs to be fixed
   • If pre-commit isn't working, follow these setup instructions
   • Fix the problems the rule detects in the codebase

5. Submit Your Changes

   • Open a Pull Request (PR) with your fixes
   • Update the PR to say Part of #XXX instead of Closes #XXX so when your PR is merged it doesn't close this issue.
   • Tag me (@RayBB) in the PR description
   • I'll review your changes and then ask staff for final approval

Need Help?

1. First, try asking ChatGPT or Claude for guidance
2. If you're still stuck, ask us in the comments - we're here to help!

Remember: The goal is to improve code quality. Take your time to understand and fix the issues properly.

Justification

Ruff has great defaults. Back when we first enabled it we chose the path of least resistance and disabled rules that weren't easy to fix right away. As we have been improving the quality of our dev tooling, re-enabling some of these rules would be great.

Breakdown

Requirements Checklist

Major Implementation (Significant Code Changes):

• ⚠️ For these tasks don't remove the rule unless you're the final PR. Otherwise the ruff rule will fail.
• PLW0602 - Audit and remove unneeded global keyword (13 occurrences) | @techy4shri
  • Enable Ruff Rule: Remove unneeded occurrences of the global keyword #10232
• SIM115 - Implement context managers for safer file handling (46 occurrences)
  • scripts/.. --> 9 occurrences | @geroferrari
    • add context manager in script folder #10265
  • openlibrary/i18n/.. --> 8 occurrences | @techy4shri
    • .
  • openlibrary/coverstore/.. --> 14 occurrences | @ananyakaligal
    • .
  • openlibrary/ --> 16 final occurrences (save this for last and enable the ruff rule) | @drona-gyawali
    • Also add a noqa statement here
    • .
• RUF012 - Review and optimize mutable class defaults (81 occurrences)
  • Task 1 (24 changes): Update worksearch schemes - openlibrary/plugins/worksearch/schemes/{works,subjects,authors}.py (8 each) | @ananyakaligal
    • (RUF012) Fixed mutable class Defaults- Task 1  #10283
  • Task 2 (20 changes): Core and schema updates - openlibrary/{tests/core/test_db.py (7), plugins/worksearch/schemes/editions.py (7), utils/schema.py (6)} | @ananyakaligal
    • (RUF012) Fixed mutable class Defaults- Task 2 #10286
  • Task 3 (13 changes): Core functionality updates - openlibrary/{plugins/worksearch/autocomplete.py (4), tests/catalog/test_get_ia.py (3), core/{edits,bookshelves}.py (3)} | @drona-gyawali
    • (RUF012) Fixed mutable class Defaults- Task 3 #10284
  • Task 4 (12 changes): Various file updates - {scripts/{providers/isbndb,partner_batch_imports}.py, openlibrary/tests/solr/{updater/test_work,test_update}.py, plugins/{upstream/mybooks,openlibrary/opds}.py} (2 each) | @drona-gyawali
    • (RUF012) Fixed mutable class Defaults- Task 4 #10278
  • Task 5 (12 changes): Single change updates - {scripts/tests/test_obfi.py, openlibrary/plugins/upstream/{forms,addbook}.py, plugins/openlibrary/tests/test_lists.py, core/{vendors,ratings,observations,models,follows,booknotes,processors/readableurls}.py, catalog/add_book/tests/test_match.py} (1 each) | @drona-gyawali
    • (RUF012) Fixed mutable class Defaults- Task 5 #10273

Requires Discussion (Staff Needed), do not ask to be assigned:

• RUF001 - Evaluate enabling ambiguous unicode character detection (add exception comments where needed)
• RUF005 - Follow standards for collection concatenation
• PIE790 - Remove empty pass statements
• UP031 - Modernize string formatting by converting %s (285 occurrences)
  • ⚠️ We need to break this down into groups of ~15 (minimize the number of files changing).
  • start UP031 by fixing simple %s strings #10266
• Add a comment on why each other the other ignores makes sense for now. | @RayBB

Completed

Spoiler

Immediate Implementation (No Code Changes Required, just remove the rule):

• PLC1901 - String type comparison rule can be enabled without modifications | @techy4shri
  • [Refactor] Removes PLC1901 - String type comparison rule can be enabled without modifications #10279
• PT004 - Deprecated pytest rule can be safely removed | @techy4shri
  • [Refactor] Removes PT004 - Deprecated pytest rule can be safely removed #10211

Minor Implementation (Simple Code Changes):

• RUF100 - Remove redundant noqa directives throughout codebase | @drona-gyawali
  • Re-enable RUF100 - Removes redundant # noqa comments  #10202
• PT006 - Standardize pytest parameter naming (can use ruff check . --unsafe-fixes) | @drona-gyawali
  • Remove PT006 - Standardize pytest parameter naming #10217
• PT021 - Enforce consistent pytest fixture naming conventions | @drona-gyawali
  • [Refactor] pytest fixture naming to comply with PT021 #10231
• PLR5501 - Convert nested if statements to elif clauses for better readability | @ananyakaligal
  • Enable PLR5501 - Convert nested if statements to elif clauses for better readability #10260
• RSE102 - Remove unnecessary parentheses in raise statements | @drona-gyawali
  • Remove unnecessary parentheses in raise statements (RSE102) #10248

Stakeholders

• @cdrini

---

Instructions for Contributors

Please run these commands to ensure your repository is up to date before creating a new branch to work on this issue and each time after pushing code to Github, because the pre-commit bot may add commits to your PRs upstream.

[drona-gyawali]

I'd like to work on rule RUF100 - Remove redundant noqa directives throughout the codebase.

[RayBB]

@drona-gyawali go for it. I put your name next to that one

[techy4shri]

I would like to work on rule B007 - Loop control variable {name} not used within loop body.

[RayBB]

Please select a rule from the list in this issue @techy4shri

[techy4shri]

PT004 has been removed, only documentation is available.
I would like to work on PT004 - Deprecated pytest rule can be safely removed. If okay then?

[RayBB]

@techy4shri go for it! Your name is on the list now!

[ananyakaligal]

@RayBB I would like to work on PLR5501 and RSE102

[RayBB]

@ananyakaligal put you down for PLR5501 as it's one at a time per the issue.

[techy4shri]

I have raised a PR for the previous rule assigned to me. Next I would like to work on PLC1901 - String type comparison rule can be enabled without modifications.

[drona-gyawali]

Hey @RayBB , with your permission, can I try working on PT006 - Standardize pytest parameter naming.

[RayBB]

@drona-gyawali yes go for it. I added your name

[techy4shri]

@RayBB I would like to work on PLW0602 - Audit and remove unneeded global keyword (13 occurrences) please!

[RayBB]

@techy4shri you're marked as working on it now. This one may be a bit tricker. Please ensure you review each instance carefully and test that nothing is broken by changing it.

[techy4shri]

I will keep that in mind, thank you!

[RayBB]

@drona-gyawali that's a good point. We can't enable the rule until all of the parts are removed. So yes we'll save that for the last task.
Additionally, I've gone ahead and split it up by file into multiple groups so it's easier for everyone else to take a section too. I went ahead and assigned you to the 5th part since it seems to has many in common with the ones you mentioned.

[ananyakaligal]

@RayBB i would like to take up the task 1 of RUF012

[techy4shri]

I would like to work on PLC1901 - String type comparison rule can be enabled without modifications @RayBB .

[RayBB]

@ananyakaligal @techy4shri you're each assigned to the issues you asked for :)

[geroferrari]

SIM115 is a large task. Please make a proposal for dividing it up by folders or parts of the code base so each part is touching less than 15 files. Then you can be assigned to one of those parts.
…
On Tue, Dec 31, 2024, 1:47 PM geroferrari @.> wrote: you are right! my bad. I would like to work on SIM115 then! — Reply to this email directly, view it on GitHub <#10196 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHA5APRJBQKUPGXFAQRM5T2ILRDVAVCNFSM6AAAAABUEFHQPGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNRWGY2TMMZXGE . You are receiving this because you were mentioned.Message ID: @.>

Proposal for dividing SIM115:

• openlibrary/i18n/.. --> 8 ocurrences
• openlibrary/coverstore/.. --> 14 occurrences
• openlibrary/"rest of files" --> 15 occurrences
• scripts/.. --> 9 ocurrences

if you agree in the division, I would like to starts with the canges within "scripts" folder

[RayBB]

@geroferrari great job with the division. This is perfect. You're assigned 👍

[drona-gyawali]

Hey @RayBB, I 'd like to work on RUF012 Task - 3

[techy4shri]

I would like to work on SIM115 - Implement context managers for safer file handling (46 occurrences) subtask openlibrary/i18n/.. --> 8 occurrences .

[RayBB]

@drona-gyawali @techy4shri I updated the list with your names for those issues.
Main issue is now up to date with all the PRs.

[geroferrari]

I am open/willing to continue adding the context manager on the following folders (SIM115),
I can continue with: openlibrary/coverstore/.. --> 14 occurrences
if you want!

[RayBB]

@geroferrari go for it and I'll ask your name next to it later :)

[ananyakaligal]

@RayBB Can I work on SIM115 - Implement context managers for safer file handling --> openlibrary/coverstore/.. --> 14 occurrences

[drona-gyawali]

Hey @RayBB , I would like to work on [ SIM115 ] openlibrary/ --> 16 final occurrences (save this for last and enable the ruff rule)

[RayBB]

@ananyakaligal @drona-gyawali I've updated the issue with all your PRs and new assignment.
With those ones assigned that wraps us up for all the ones we can do now. I'll have to wait until discussing with staff for the others and then ping you. Overall, great work on these!