Convert the Bibliotheca integration into a package

[jonathangreen]

Description

Restructures the Bibliotheca integration from five flat bibliotheca_* modules into a bibliotheca/ package, matching the layout of every other integration.

bibliotheca.py (1295)                    bibliotheca/
bibliotheca_importer.py            →       api.py                      596
bibliotheca_purchase_record_...py          parser.py                   621
bibliotheca_circulation_updater.py         coverage.py                  65
bibliotheca_scripts.py                     settings.py                  57
                                           constants.py                 13   (new)
                                           importer.py                 212
                                           purchase_record_importer.py 217
                                           circulation_updater.py      288
                                           scripts.py                  158

The 1295-line bibliotheca.py is split along the seams the siblings already use: settings.py (as in overdrive/ and boundless/), parser.py (as in boundless/), and coverage.py (as in overdrive/).

A cycle had to be broken first. BibliothecaAPI constructs the parsers, but ErrorParser, EventParser and PatronCirculationParser reached back for BibliothecaAPI.SERVICE_NAME and .ARGUMENT_TIME_FORMAT. The new constants.py holds those values and the parsers read them from there — the same way boundless/parser.py and overdrive/coverage.py already avoid this. BibliothecaAPI keeps both class attributes (tests and license_providers.py read them off the class), so no caller changes.

Two dead attributes removed. EventParser.EVENT_SOURCE has no readers anywhere in the repo. EventParser.SET_DELIVERY_MECHANISM_AT also has none: the only two read sites (api/circulation/dispatcher.py and feed/annotator/circulation.py) access it off a BaseCirculationAPI, and EventParser is an XMLProcessor. Dropping them lets parser.py stop importing the circulation API layer entirely. BibliothecaAPI.SET_DELIVERY_MECHANISM_AT is a separate attribute on a separate class and is unchanged.

Tests mirror the new layout, with the shared fixture moved into a conftest.py as boundless/ does. TestBibliographicCoverageProvider subclassed TestBibliothecaAPI while using nothing from it, so pytest was silently re-running all 22 API tests a second time — that inheritance is gone. All 38 declared tests survive; test_internal_formats moves to TestItemListParser, the class it actually exercises.

Motivation and Context

src/palace/manager/integration/license/ held bibliotheca_api.py-style flat modules for Bibliotheca while overdrive/, boundless/ and opds/ were all packages. This is a structural cleanup only — no behavior change.

How Has This Been Tested?

• mypy clean across all 1171 source files.
• All pre-commit hooks pass.
• Bibliotheca + Celery task suites: 167 passed.
• Every external test file importing Bibliotheca (test_loan, test_dispatcher, test_collection, test_monitor, test_configuration, service/, …): 522 passed.
• The two files that actually consume SET_DELIVERY_MECHANISM_AT (test_dispatcher.py, test_library_annotator.py) run green after the attribute removal.
• All six bin/ scripts had their imports statically resolved against the new module paths.
• Verified mechanically that the split moved code rather than changed it: the class inventory and the member inventory (defs, decorators, constants) diff empty against the original file apart from the four intended constant substitutions. Likewise the set of declared Class::method test pairs diffs empty apart from the intentional test_internal_formats relocation.

Checklist

• I have updated the documentation accordingly.
• All new and existing tests passed.

[claude]

Claude finished @jonathangreen's task in 4m 59s —— View job

---

Code Review: Convert the Bibliotheca integration into a package

• Gather context (diff, new package modules, tests)
• Verify the "move-only" claim for the split modules
• Check the constants.py cycle break & class-attribute preservation
• Validate the dead-attribute removals
• Review test reorganization
• Post review

Summary

This is a clean, faithful move-only refactor, and I found no blocking issues. I independently verified the load-bearing claims rather than taking them on faith:

• Constant substitutions preserve values. BIBLIOTHECA_SERVICE_NAME = "Bibliotheca", BIBLIOTHECA_TIME_FORMAT = "%Y-%m-%dT%H:%M:%S", and BIBLIOTHECA_LABEL = DataSourceConstants.BIBLIOTHECA — each matches the class attribute / inline literal it replaces (SERVICE_NAME, ARGUMENT_TIME_FORMAT/INPUT_TIME_FORMAT, and label()). BibliothecaAPI still carries SERVICE_NAME, ARGUMENT_TIME_FORMAT, and SET_DELIVERY_MECHANISM_AT = None for its off-the-class callers.
• The two removed attributes are genuinely dead. A repo-wide search finds no reader of EventParser.EVENT_SOURCE (only overdrive/api.py defines its own), and every SET_DELIVERY_MECHANISM_AT read site (api/circulation/dispatcher.py:206, feed/annotator/circulation.py:1244) accesses it off a BaseCirculationAPI, never off the EventParser. Dropping them lets parser.py avoid importing circulation/base.py, while still legitimately importing circulation/data and circulation/exceptions.
• No stale import paths remain. Searched all of src/ and tests/ — every reference now targets bibliotheca.api / .parser / .importer / .circulation_updater / .purchase_record_importer / .scripts / .coverage. The bin/ scripts, the registry lazy import, scripts/availability.py, and the celery task all resolve. The pre-existing local imports (to break the circulation_updater ↔ api and Celery-autoload cycles) are retained with updated paths.
• Test inventory matches exactly. All 38 declared tests survive; test_internal_formats correctly moves from TestBibliographicCoverageProvider to TestItemListParser (the class it exercises). I also confirmed the original TestBibliographicCoverageProvider(TestBibliothecaAPI) inheritance — which silently re-ran the whole API suite a second time — is gone, so the split is a net correctness win for the test run.

The bibliotheca/__init__.py being empty (imports go through the submodules) matches the existing overdrive/ and boundless/ package conventions. I could not execute the test suite or mypy in this environment (Python execution requires interactive approval that wasn't available), so I relied on static verification plus the diff — but the import graph, constant equivalences, dead-code analysis, and test-method census all check out, consistent with the green CI reported on the PR.

Nice cleanup — no changes requested.
· chore/bibliotheca-package

[greptile-apps]

Greptile Summary

This PR moves the Bibliotheca integration into a package layout. The main changes are:

• Bibliotheca API, parser, settings, coverage, importer, updater, and script code split into package modules.
• Internal imports updated to use the new submodule paths.
• Shared constants moved into a dedicated constants module.
• Tests reorganized to match the new package structure.

Confidence Score: 5/5

This looks safe to merge.

• No blocking issues found in the changed code.

Important Files Changed

Filename	Overview
src/palace/manager/integration/license/bibliotheca/api.py	Moves the Bibliotheca API implementation into the new package while preserving the class-level constants used by callers.
src/palace/manager/integration/license/bibliotheca/parser.py	Moves the XML parser classes into the package and reads shared service constants from the constants module.
src/palace/manager/celery/tasks/bibliotheca.py	Updates Celery task imports to the new Bibliotheca package modules.
src/palace/manager/service/integration_registry/license_providers.py	Updates the Bibliotheca provider registration to import the API from the new module path.

_{Reviews (3): Last reviewed commit: "Update stale test module cross-reference..." | Re-trigger Greptile}

[codecov]

Codecov Report

❌ Patch coverage is 88.90675% with 69 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.48%. Comparing base (87d1bfa) to head (c2b3695).
⚠️ Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
...ace/manager/integration/license/bibliotheca/api.py	84.53%	37 Missing and 6 partials ⚠️
.../manager/integration/license/bibliotheca/parser.py	92.75%	13 Missing and 8 partials ⚠️
...anager/integration/license/bibliotheca/coverage.py	85.18%	2 Missing and 2 partials ⚠️
src/palace/manager/scripts/availability.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3543      +/-   ##
==========================================
+ Coverage   93.47%   93.48%   +0.01%     
==========================================
  Files         512      516       +4     
  Lines       46619    46673      +54     
  Branches     6353     6359       +6     
==========================================
+ Hits        43575    43632      +57     
+ Misses       1968     1966       -2     
+ Partials     1076     1075       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jonathangreen]

The coverage issues here are preexisting, and the overall coverage doesn't change. So I think this one is ready for a review. I tagged you @dbernstein since you did the recent Bibliotheca refactoring.