Split cli.py into a command package - #19
Merged
Merged
Conversation
The import list in cli.py will look like a formality to anyone who reads it, and "why not scan the directory?" is the first idea a reader has when they see nine import lines. Both alternatives were considered and rejected for reasons that are not visible from the code itself. The working spec that weighs them is not part of this repository and is archived once the work is done, so the reasoning goes here instead.
Spec 001 moves every command into nine modules that cli.py imports by name. A module left out of that list registers nothing, and the CLI still starts — it is simply missing commands. The same holds for a second Command Name left behind when its Command moves to another module. The README counter test does go red on a missing module, but it reports a sum, not a name, and it excludes Command Names that are not canonical: with `delete-block` unregistered it stays green. Measured both ways before writing this: dropping the alias registration leaves the README tests passing and makes this one name `delete-block` in its failure. The set is literal rather than derived. Iterating cli.commands would assert that the registry equals itself, and a module that is never imported leaves nothing to iterate over; a count holds until something is added in the same commit and then does not say what went missing.
The wrapper copied __name__ and __doc__ by hand. That is enough for the help text, and not enough for anything that asks where a callback came from: __module__ stays that of the module defining the decorator, and __wrapped__ is never set. Today decorator and commands share one file, so the two cannot be told apart. Spec 001 moves the decorator to output.py and the commands to nine modules, and tests/test_dry_run_coverage.py unwraps each callback and parses the module __module__ names to decide which commands write. Measured in a full simulation of that layout: without wraps the scan reports 0 writing commands instead of 18, silently, because every callback claims to live in the decorator's module. Probe, red before and green after: decorate a function defined in __main__ and read back __module__ and __wrapped__. Click does not inspect callback signatures, so the CLI is unchanged — both help baselines diff empty.
The scan that decides which commands write read cli.py as text and cut each body at the next top-level def. That puts the decorator lines of the following function at the end of the previous body: 73 of them carry foreign trailing text today. It changes nothing right now — regex and ast agree on the same 18 writers — but it holds only while the neighbours stay put, and spec 001 moves every command into one of nine modules. The failure would have been silent. The file's own guard checked five example names, so a scan that found fewer commands than it should would still pass, and --dry-run coverage across 18 commands would go unchecked. So: parse with ast, per module, keyed on the callback's __module__ after unwrapping; and assert the full set of writers as an equality, so a scan that shrinks and an inventory that shrinks both fail. Both directions probed and restored: removing --dry-run from create-page names it in the failure, and dropping a name from the known set fails too.
The test reads every module except api.py into `code` and then asserts only against `api_src`. Verified with ast: the function stores `code` and never loads it. It came in with a5f2c19, which removed a cacheable method nobody called; the scan looks like it was meant to check call sites and then was not. Removed rather than repaired. Making it recursive and asserting against it would turn a dead line into a new assurance, which belongs with the _MUTATING_METHODS work the roadmap files alongside spec 010. Left as it is, it would get worse: src.glob("*.py") is not recursive, so once spec 001 moves the commands into logseq_cli/commands/ the line would look like it covered them while covering nothing.
First extraction of spec 001. handle_connection_error, output and fail go across unchanged; cli.py imports them back, so nothing that calls them moves yet. The imports the new module needs were read off the moved code with ast rather than guessed: click, requests, functools, json, sys, DatalogQueryError, ConfigError, InvalidKeywordError. Three of those then had no reader left in cli.py and are removed there. Three imports in cli.py are unused and stay: block_uuid_from_result, escape_regex and has_flush_newline_bullets were already dead before this commit and are not this commit's to clean up. Suite unchanged at 833, both help baselines diff empty, audit script exit 0.
Spec 001 gives render.py, group.py and nine command modules their own files. These nine names are read from outside the module that defines them, and a private name imported from three other modules is not private — the underscore would misdescribe it. Nothing moves here. A rename and a move in one commit is two changes with one diff that shows neither: git log --follow and blame lose the thread, and a reviewer cannot tell a renamed line from a moved one. Keeping them apart is what lets every later commit in the series be a pure move. _resolve_single_ref keeps its underscore: only resolve_refs_in_blocks calls it, inside the same module. tests/test_version.py follows the rename in this commit because it imports the name; its module changes in the step that moves resolve_version to group.py.
Nine symbols, unchanged: the two reference resolvers, the four that render blocks to text or Markdown, the section extractor, the properties predicate and BLOCK_REF_RE. cli.py imports them back, so no caller moves yet. pages, journal and todos all read these, which is why they get their own module rather than living with whichever command module happens to use them most. _resolve_single_ref comes along and keeps its underscore — it is called only from resolve_refs_in_blocks, in the same file. The module needs exactly two imports, read off the moved code rather than guessed: re, and normalize_heading from helpers. Suite 833, both help baselines diff empty, audit script exit 0.
resolve_version and the group callback go across unchanged. cli.py imports both back, so it still exposes `cli` and nothing that decorates against the group has to move yet. This is the commit that moves the API constructor out of cli.py, so the suite follows in the same commit: 220 occurrences of logseq_cli.cli.LogseqAPI across 38 files become logseq_cli.group.LogseqAPI. Counted before and after — a sed over 38 files reports nothing on its own — and both counts are 220 with no occurrence of the old target left. mock.patch replaces a name in the namespace it is given, so a patch left on cli.py would have replaced a re-exported name nobody calls and built a real client against a mock server. Probed rather than assumed: pointing one file's patches back at logseq_cli.cli turns 8 of its 12 tests red, so the new target is the one doing the work. tests/test_version.py changes module here (its rename happened in the previous commit): resolve_version now comes from logseq_cli.group. Left on cli.py it would have stayed green until the meta.py commit and then failed with an ImportError at a place no table names. resolve_version stays one level below the repository root because it finds pyproject.toml relative to its own file; group.py is that level. Suite 833, both help baselines diff empty, audit exit 0.
First of the nine command modules, and the one that creates the subpackage — smallest and most isolated first, so the pattern is proven before the large ones. Registration happens as a side effect of the import in cli.py; the commands themselves are unchanged. The packaging line belongs in this commit, not after the series: setuptools does not infer subpackages from an explicit `packages` list, so from here on every wheel without it would ship a CLI that installs, starts, and has no commands. `pip install -e .` hides this completely, and so does `pip wheel` reading a cached build — measured. Checked with --no-cache-dir against the source tree: 11 modules in the wheel, 11 in source. tests/test_find_block_children.py imports FIND_BLOCK_CHILDREN_LIMIT and moves with it. Its import is the combined form, so the whole line is replaced rather than the symbol. Suite 833, both help baselines diff empty, audit exit 0.
Six symbols: the two commands, the marker table and the three helpers that only they use. set-todo-status matches block references, so this module imports BLOCK_REF_RE from render.py — one of the two cross-module edges the map records, and the reason that regex is not private to either side. No test pointer moves with it. Suite 833, both help baselines diff empty, audit exit 0.
Five commands and the four helpers only they use. tests/test_property_list_values.py imports _format_property_value and moves with it; its import is the combined form, so the whole line is replaced. Suite 833, both help baselines diff empty, audit exit 0.
The command and the two helpers only it uses. No test pointer moves with it. Suite 833, both help baselines diff empty, audit exit 0.
Twelve symbols: the two commands, the config renderer and the helpers only they use. doctor reports the version, so this module imports resolve_version from group.py — the second of the two cross-module edges the map records. Three test pointers move with it. Two are dotted patch targets; the third is tests/test_doctor.py taking the module object itself to replace import_module. That third one does not fail the way the spec predicted. Pointing it back at logseq_cli.cli does not raise AttributeError — cli.py still imports import_module for its own use, so monkeypatch.setattr succeeds and patches a name doctor no longer reads. The test fails on its assertion instead, which is loud enough here only because it asserts the patched behaviour directly. Suite 833, both help baselines diff empty, audit exit 0.
Four commands and the four helpers only they use. Four test pointers move with it, two of each kind. The imports of _project_pattern, _word_pattern and _is_incidental_page fail loudly if left behind; the two get_page_content patches do not — the helper is defined in helpers.py and read here, so a patch left on logseq_cli.cli would replace a name nobody calls and let the real helper run against the mock API. Patching it at its definition does not work either, and an earlier revision of the spec prescribed exactly that: pointing this file at logseq_cli.helpers.get_page_content turns 7 of its 13 tests red. mock.patch replaces a name in the namespace it is given, and the namespace that reads it is this module. tests/test_config_integration.py used the combined import form and takes `cli` from logseq_cli.cli, not from the command module — a test takes the group from the entry point, so that the registry it sees is the full one. Suite 833, both help baselines diff empty, audit exit 0.
Nine commands and the backlink-context helper. tests/test_backlinks_context.py imports that helper and moves with it. It is the fifth file using the combined import form, and the second where the two names are the other way round, so the whole line is replaced rather than the symbol. Suite 833, both help baselines diff empty, audit exit 0.
Seven commands, and the alias registration that sits outside every map entry: delete-block is added with cli.add_command a few lines below remove-block rather than by decorator, so it travels with the command it names or it stays behind referring to a symbol cli.py no longer defines. Probed here rather than trusted: commenting out that one line makes the registry guard from the start of this series fail naming delete-block. Suite 833, both help baselines diff empty, audit exit 0.
Five commands, the last of the nine modules. The two remaining get_page_content patches move with it, for the same reason as the analysis ones: the name is read here, and a patch left on cli.py would have replaced something nobody calls without turning anything red. With this commit the test migration is complete: no test reaches into logseq_cli.cli for anything but the group object, on any of the three forms the acceptance rule greps for. Suite 833, both help baselines diff empty, audit exit 0.
Everything has moved, so what is left is the import list that makes the commands exist and the main() the console script points at: 5390 lines down to 33. The 70-odd imports the file still carried were read by nothing. The largest file in the package is now commands/journal.py at 820 lines, and logseq_cli/ holds nine command modules plus group, output and render. Wheel checked against the source tree with --no-cache-dir: 19 modules in both. Suite 833, both help baselines diff empty, audit exit 0.
pyproject.toml lists packages explicitly, and setuptools does not infer subpackages from an explicit list. So logseq_cli/commands could be left out of a release without anything going red: the tests run against an editable install, which links the source tree and cannot see the difference. What reaches a user is a CLI that installs, starts, and has no commands. The job compares the wheel against the source tree rather than looking for logseq_cli/commands by name, so a later subpackage is covered without touching this file. Two flags earn their place, both measured while writing this. Without --no-cache-dir, pip serves a cached build and the check inspects a wheel it did not build. With a stale logseq_cli.egg-info in the tree, setuptools reuses its SOURCES.txt and ships the subpackage even after `packages` was narrowed — so the clean step comes first. With both in place, reverting the packages line fails the check and names all ten missing modules.
Both structure trees named four modules and one cli.py "with all commands". They now list group, output, render and the nine command modules — and config.py, which was missing from both before this work started. CONTRIBUTING told a reader to start in cli.py, "over five thousand lines, which is more than one file should carry and is being split". That file is 33 lines now, so the sentence points at the wrong place twice. It is replaced by what a contributor actually needs: which module a command lives in, that a new module has to be named in the import list or its commands do not exist, and which test says so by name when it is forgotten. Neither tree carries a command count or a line number. Both would drift, and the registry is the authority anyway. Re-checked against the built state rather than assumed: the ADR's three claims (empty commands/__init__.py, group.py importing nothing from commands/, cli.py naming all nine modules) hold; CONTEXT.md makes no claim about file structure; 38 command names over 37 callbacks; every --page option still has its --name alias; check-links reports 8 files, 0 broken links; suite 833.
Two entries. The Changed one says what a user of the package needs: nothing about the CLI changes, and the two parts of the work that are not pure text moves are named rather than folded into "refactoring". The Fixed one is worded for what actually happened. `packages = ["logseq_cli"]` was correct for a flat package and only became wrong once there was a subpackage, which landed in the same commit as the fix — so it ships something new rather than repairing something that used to fail. No release was affected. The CI job that checks this gets no entry: it changes nothing a user can observe.
The wrapper was given functools.wraps earlier in this series because the --dry-run scan unwraps each command callback and parses the module __module__ names. Nothing asserted that property: at the commit that introduced it, taking functools.wraps back out again left all 833 tests green. It only becomes checkable one commit later, through a test about --dry-run coverage. That test does notice, but it cannot say why. It reports that create-page and sixteen others write without offering --dry-run, which reads as a defect in the commands rather than in the decorator two files away. Four assertions instead: the wrapper keeps the wrapped function's module, it exposes __wrapped__, it still carries name and docstring, and every callback in the registry unwraps to its own module rather than to logseq_cli.output. Probed by removing functools.wraps again: three of the four go red and name the cause. The fourth stays green, correctly — the hand-built wrapper copied __name__ and __doc__ all along, and that was exactly what made the gap look like it was covered.
The registry assertion in this file is supposed to catch a command module that cli.py stops importing. Run on its own it does. In the full suite it does not, for four of the nine: test_find_block_children.py, test_property_list_values.py, test_config_integration.py and test_backlinks_context.py import logseq_cli.commands.blocks, .properties, .analysis and .pages directly to reach a helper, and importing a command module registers its commands as a side effect. By the time the registry is asserted, another file has filled it. Measured by dropping each of the nine from the import list in turn: blocks, properties, analysis and pages left all 837 tests green. The other five failed loudly, but only because their commands were exercised elsewhere — nothing was asserting the import list. So assert it: parse cli.py and compare the names it imports against the modules on disk. That holds regardless of what any other test imported first. With it, all nine removals fail.
ADR 0001 states three rules the split rests on: a command module decorates against the group, commands/__init__.py stays empty, and group.py imports nothing from commands/. Until now only one of them failed on its own — the cycle, loudly. The other two were quiet: a command module reaching sideways for a neighbour's helper works until two of them reach for each other, and an __init__.py that imports a module makes it register whether or not cli.py names it, which turns the explicit import list into decoration and hides the very omission the registry test exists to catch. Both were checked during the split by local/specs/audit-001-map.py. That script parses the module map out of a specification that is now archived, and it runs in no test and no CI job. A rule enforced only by a tool nobody runs is a rule on paper. Four assertions against the source, plus the layering rule the ADR implies: nothing below the command layer imports from commands/. Each probed by breaking it — sideways import, non-empty __init__, group.py reaching down, render.py reaching up. All four go red; the cycle as a collection error, which is louder still.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Split cli.py into a command package
What changed
cli.pyheld every command in 5390 lines. The commands now live inlogseq_cli/commands/, one module per group, with three modules beside them:group.py— the click group, the global options, and the only place the APIclient is constructed
output.py— results on stdout, failures on stderr,--jsonrender.py— blocks to text, and resolving block referencescli.pyis the entry point that imports them: 33 lines. A command exists onceits module has been imported, and that list is written out rather than
discovered by scanning —
docs/adr/0001-explicit-command-registration.mdsayswhy, and records the two alternatives that were rejected.
What did not change
The console entry point, every command name, every option, every default and
every help text. The per-command
--helpoutput of all 38 command names wascaptured before the first commit and diffed against after each of the 22, and
it never differed once.
How it is verifiable
Nineteen of the 22 commits are text moves with the suite green at each, so the
series bisects. Four mid-series commits were checked out and run to confirm
that rather than assume it.
Three commits are not moves and are separated on purpose:
a commit where nothing else happens
handle_connection_errorbuilds its wrapper withfunctools.wraps--dry-runguarantee is parsed withastinstead ofsplit on top-level
defThe last two are the interesting ones. The
--dry-runcontract covers 18commands, and the scan that enforces it found them by reading
cli.pyas text.Measured in a full simulation of the target layout before any of this was
committed: without
functools.wraps, that scan finds 0 writers instead of18 — and reports it by passing.
Added
tests/test_command_registry.pyasserts the full set of 38 command names. Acommand module nobody imports registers nothing and the CLI still starts;
the README counter test does notice, but it reports a sum, not a name, and it
stays green when the
delete-blockalias is the thing that went missing.Both cases probed.
pyproject.tomllists packages explicitly and setuptools does not infer a subpackage from an
explicit list, so a release could have shipped a CLI that installs, starts,
and has no commands.
pip install -e .cannot see this.Three guarantees that nothing was asserting
Found by mutation testing at the end of this work — change one thing, run the
whole suite, see whether anything goes red. Each of the three is a rule this
split introduces, so none of them could have been broken before it.
functools.wraps: at the commit that adds it, taking it back out againleft all 833 tests green. It only becomes checkable one commit later, through
a test about
--dry-runcoverage that reports the symptom rather than thecause. Asserted directly now.
cli.pyleft the suitegreen for four of the nine. Other test files import
commands.blocks,.properties,.analysisand.pagesdirectly to reach a helper, andimporting a command module registers its commands as a side effect — so the
registry had been filled by somebody else before the guard looked at it. The
import list is now read from the source, which does not depend on what ran
first. All nine removals fail with it.
neighbour's helper, and a non-empty
commands/__init__.py, both passed. Theywere checked only by
local/specs/audit-001-map.py, which parses a spec thatis now archived and runs in no CI job. Four assertions in
tests/test_package_layering.pyinstead, each probed by breaking it.Checks
green at every commit
logseq_cli.clifor anything but the group objectLogseqAPI(appears on exactly one line, ingroup.pyoutside the source tree it reports 38 commands and the right version
CHANGELOG
Two entries under Unreleased: the split under Changed, and the packaging line
under Fixed — worded as shipping the subpackage rather than repairing a
defect, because the list was correct while the package was flat and no release
was affected.