Fix missing f-string in facet_wrapper assert message (#12649)#12691
Conversation
|
Thanks for your contribution, @tranmin001! Welcome to Open Library — this appears to be your first PR here. 🤖 Copilot has been assigned for an initial review. The linked issue (#12649) hasn't been triaged yet — triage happens on Mondays and Fridays. There are currently 73 open non-draft PRs ahead of yours in the review queue. PR triage checklist (maintainers / Pam)
Note This comment was automatically generated by Pam, Open Library's Project AI Manager, on behalf of @mekarpeles. Pam is designed to provide status visibility, perform basic project management functions and relevant codebase research, and provide actionable feedback so contributors aren't left waiting. |
There was a problem hiding this comment.
Pull request overview
Fixes an unhelpful assertion message in the WorkSearch subject facet wrapping logic by converting the string to an f-string so the failing facet value is included, and adds a regression test to ensure the assertion message contains the actual facet value.
Changes:
- Update
facet_wrapper’s assertion message to interpolatefacetcorrectly. - Add a new pytest regression test that triggers the assertion and checks the error message includes the facet value.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| openlibrary/plugins/worksearch/subjects.py | Fixes the assertion message to include the invalid facet value via f-string interpolation. |
| openlibrary/plugins/worksearch/tests/test_subjects.py | Adds a regression test for the assertion message content when the facet engine lookup fails. |
Closes #12649
[fix]
Technical
Adds the missing
fprefix to the assert message string infacet_wrapperinopenlibrary/plugins/worksearch/subjects.pyline 337. Without the fix, the error message always printed the literal string{facet}instead of the actual facet value, making the assertion unhelpful for debugging.This is my first open source contribution, working on it as part of a university course at UW Bothell. Open to any feedback or change!
Testing
Added
openlibrary/plugins/worksearch/tests/test_subjects.pywith a test that patchesSUBJECTSto an empty list to trigger the assert, then verifies the error message contains the actual facet value rather than the literal string{facet}.To run:
sudo docker compose run --rm home pytest openlibrary/plugins/worksearch/tests/test_subjects.py -vRan
make testlocally — 3780 tests passed, 1 failed. New test file: 1 passed, 0 failed.OLMarkdownEditor.test.jsfails due to a missing@tiptap/coredependency, pre-existing and unrelated to this change.@mekarpeles