Skip to content

Say what the core/ and scripts/ deprecations actually mean - #3749

Open
dbernstein wants to merge 1 commit into
mainfrom
chore/correct-deprecated-package-guidance
Open

dbernstein wants to merge 1 commit into
mainfrom
chore/correct-deprecated-package-guidance

Conversation

@dbernstein

Copy link
Copy Markdown
Contributor

Description

Replaces the bare deprecated - no new code markers on /core and /scripts in CLAUDE.md with what the deprecations actually mean.

   - `/core` - Legacy miscellaneous components (**deprecated - no new code**)
+    - Exception: `/core/classifier` is the active home of classification logic (the BISAC
+      rulesets and code table, keyword matching, `WorkClassifier`). It has no replacement
+      elsewhere in the tree, so classification changes belong here.
...
-  - `/scripts` - Legacy CLI utilities (**deprecated - no new code**)
+  - `/scripts` - Legacy CLI utilities (**deprecated - put new logic in `/celery/tasks`**)
+    - The package is deprecated for *logic*, not for entry points. A thin `Script` subclass
+      here plus a `bin/` wrapper is still the only supported way to run something on demand,
+      and is still the expected pattern for dispatching a Celery task by hand. There is no
+      replacement framework: `palace-startup-task` is the only console script the project
+      ships, and it resolves into this package.

Documentation only. No code changes.

Motivation and Context

Both markers arrived together in 21df3c5d9 (2026-03-16), the commit that created the Claude Code config files. They were written as scaffolding rather than as separately-argued policy, and read literally neither is accurate. AI reviewers act on the literal reading and ask for moves that have nowhere to go — this has now fired on three consecutive PRs (#3726, #3736, #3737), each time citing CLAUDE.md as the authority and each time asking to "move this to the supported framework."

/scripts — there is no replacement framework

[project.scripts] entries one: palace-startup-task = "palace.manager.scripts.startup:create_startup_task" — it resolves into the deprecated package
cli/ package, click/typer app, palace command none exist
bin/ wrappers importing palace.manager.scripts directly 50 of ~74
...reaching it by subclassing Script / CollectionInputScript / InputScript 16 more, via the integration packages
the only exception palace.manager.customlists, a purpose-built argparse CLI for custom-list import/export

scripts/base.py also imports TimestampData from palace.manager.core.monitor, so the deprecated CLI framework is built on the deprecated core package.

The rule is not being followed either, which is the clearest evidence it is mis-stated. Since 2026-03-16 five new Script subclasses have landed — UpdateExpiredLicensesScript, OverdriveReaperScript, ReclassifyNullAudienceWorksScript, CustomListEntriesSweepScript, ResetNonBisacNonfictionSubjectsScript — along with five new bin/ wrappers.

The startup-task framework is not a substitute: it runs once per deployment and has no on-demand path. The repo treats the two as complementary and ships them together — 9c626b229 added a startup task, a Script and a bin/ wrapper in one commit for the null-audience repair.

What the deprecation is really protecting is business logic, and that convention is being followed: the logic lives in /celery/tasks, and the class here is a few lines that call .delay(). The wording now says that.

/core — true in general, false for the classifier

core/ excluding the classifier is net −2,001 lines over the last 12 months. It is actively being drained:

commit what came out
7a3274ea1 core/coverage.py, −1,364 (CoverageProvider machinery, PP-4468)
963b651b2 core/monitor.py, −230
49bc00071 core/equivalents_coverage.py −201, core/query/coverage.py −58
17b873e4d core/facets.py, −168

core/classifier/ over the same period is net +131, and is the only part of core/ still receiving behaviour changes (7d5b8a06a, ff2432b7e, and the PP-4849 work). There is no candidate home for it: packages/ contains only palace-opds and palace-util, neither with any classification code, and palace/manager/service/ is dependency-injection wiring. 23 modules across the tree import from palace.manager.core.classifier; all are consumers.

So the general rule stands and the exception is now stated, rather than left for each reviewer to rediscover.

How Has This Been Tested?

Documentation only — no code paths touched. pre-commit run --files CLAUDE.md passes, including PyMarkdown.

Checklist

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

🤖 Generated with Claude Code

Both markers were written as "deprecated - no new code" when CLAUDE.md
was first added in 21df3c5. Read literally, neither is true, and
reviewers acting on the literal reading keep asking for moves that have
nowhere to go.

`scripts/` has no replacement. `[project.scripts]` holds exactly one
entry and it resolves into that package; there is no cli/ package, click
app or `palace` command; and of the ~74 wrappers in bin/, 50 import
palace.manager.scripts directly and 16 more subclass Script through the
integration packages. Five new Script subclasses and five new bin/
wrappers have landed since the marker was written. What the deprecation
is actually protecting is business logic, which now goes in
celery/tasks/ with a thin dispatcher here.

`core/` is genuinely being drained -- net -2,001 lines over the last
year, as coverage providers, monitors and facets came out. But
`core/classifier/` is net +131 over the same period and is the only part
still taking behaviour changes. Nothing outside it defines classification
logic, and there is no candidate home: packages/ holds only palace-opds
and palace-util, and service/ is DI wiring.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@claude

claude Bot commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

Claude finished @dbernstein's task in 1m 51s —— View job


Claude Code Review

  • Read the PR diff
  • Verify the /core/classifier claims against the tree
  • Verify the /scripts claims (console scripts, bin/ wrappers, Celery dispatch pattern)
  • Post review

Summary

Documentation-only, and the substantive claims hold up against the tree: palace-startup-task is indeed the sole [project.scripts] entry across the workspace and it resolves into palace.manager.scripts.startup; core/classifier/ really does hold the BISAC rulesets/code table (bisac.py), keyword matching (keyword.py, 1274 lines) and WorkClassifier (work.py:13) with no candidate home elsewhere; and the "thin subclass that dispatches a task" pattern is real (12 modules under scripts/ call .delay()/apply_async(), e.g. bin/custom_list_entries_sweep). Replacing the bare markers with the actual policy is a clear improvement. My only concerns are two over-broad phrasings — which matter more than usual here, since the stated purpose of the change is to keep literal readers from acting on imprecise wording.

Details

Minor: CLAUDE.md:71-73

The claim that a Script subclass plus a bin/ wrapper is "the only supported way to run something on demand" is contradicted by the /customlists bullet five lines above: bin/customlist_import and bin/customlist_export import palace.manager.customlists.importer / .exporter directly and never touch palace.manager.scripts. The PR description itself lists palace.manager.customlists as "the only exception," but that carve-out didn't make it into the doc — so a reviewer reading this line literally (exactly the failure mode the PR is fixing) could push a custom-list CLI change into /scripts. Suggest naming the exception, e.g. ...is the standard way to run something on demand (the one exception is /customlists, which ships its own argparse CLI).

circulation/CLAUDE.md

Lines 71 to 73 in 6c5eb6a

- The package is deprecated for *logic*, not for entry points. A thin `Script` subclass
here plus a `bin/` wrapper is still the only supported way to run something on demand,
and is still the expected pattern for dispatching a Celery task by hand. There is no

Minor: CLAUDE.md:65

"so classification changes belong here" is broader than the exception it's justifying — the preceding sentence scopes the exception to classification logic, but this clause covers all classification changes. Classification orchestration already lives in /celery/tasks/work.py (reclassify_null_audience_works, classify_unchecked_subjects) and the classification ORM model in /sqlalchemy/model/classification.py, so read literally this would invite a new classification task into a deprecated package. Narrowing it to match the sentence before keeps the exception tight:

      elsewhere in the tree, so changes to the classification logic itself belong here.
      Classification tasks still go in `/celery/tasks`.

elsewhere in the tree, so classification changes belong here.

· branch chore/correct-deprecated-package-guidance

@greptile-apps

greptile-apps Bot commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 4/5

The documentation-only change appears safe to merge, with a non-blocking clarification recommended for the on-demand CLI wording.

Findings

  1. P2 On-demand CLI claim is overbroad

Summary

Clarifies the deprecation guidance in CLAUDE.md without changing executable code.

  • Identifies core/classifier as an active exception to the general core deprecation.
  • Preserves thin operational entry points in scripts while directing new task logic to celery/tasks.
  • One non-blocking wording correction would preserve the specialized custom-list CLI exception.

Reviews (1) · Last reviewed commit: "Say what the core/ and scripts/ deprecat..."

Comment thread CLAUDE.md
- `/scripts` - Legacy CLI utilities (**deprecated - no new code**)
- `/scripts` - Legacy CLI utilities (**deprecated - put new logic in `/celery/tasks`**)
- The package is deprecated for *logic*, not for entry points. A thin `Script` subclass
here plus a `bin/` wrapper is still the only supported way to run something on demand,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 On-demand CLI claim is overbroad

“The only supported way to run something on demand” overlooks bin/customlist_import and bin/customlist_export, which invoke their dedicated implementations directly without a Script subclass or Celery dispatch. This wording could prompt the same unnecessary migration requests this clarification aims to prevent. Describe this as a supported general-purpose pattern, or explicitly preserve the specialized custom-list CLI exception.

Suggested change
here plus a `bin/` wrapper is still the only supported way to run something on demand,
here plus a `bin/` wrapper is still a supported way to run something on demand,

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant