Repository navigation
Conversation
…rgv; docs sweep Names, labels, paths and attribute values are escaped before Rich prints them, so view, ls, split and every message show them as the store spells them (F042); convert --help keeps pyadata-cli[spatial] and [pickle] (F160). view on a missing entry fails on stderr with exit 1 (F096). The stderr console no longer forces a terminal, so a log holds no colour or cursor codes and an error stays on one line (F151). main(argv) and main_deprecated(argv) run the arguments given (F161). Help and reference contradictions fixed (F159); AGENTS.md, developers.md, testing.md, troubleshooting.md, pipelines.md and STATUS.md brought up to date (F149, F150, F155, F157). The docs test now also checks that every test file is in testing.md's map and every option in commands.md. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s --chunk Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
…, docs - util/markup.literal replaces rich.markup.escape everywhere: escape doubles a trailing backslash, which printed twice wherever text followed. - split's header escapes the whole obs[column]: a column with an unclosed bracket closed into a tag on the template's own ]. - main(argv) raises SystemExit for its caller; only the command itself ends the process at once on failure. - Docs: usage errors are Typer's box, not an Error: line; the held-whole lists gain JSON and --in-memory; export sparse's --chunk row; testing.md's job names and the rank_genes_groups row; AGENTS.md's CI list and the click note; developers.md in step. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Contributor
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
testing.md: B10's job table with B6b's cost-guards job, the unit job's marker, and a row for test_block_layout.py. troubleshooting.md: B10's list of what is held whole with B6b's shard-writing bound. AGENTS.md and developers.md name the cost-guards job. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
typer 0.21 made Rich markup the default for help text; 0.20, the floor, printed it raw. So on the floors job convert --help showed 'pyadata-cli\[spatial]' with its escape, and the F160 test failed there. Setting rich_markup_mode on the app and both sub-apps gives the same help on every typer the floor allows (checked on 0.20.0, 0.21.1 and 0.27.3). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Merged
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
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.
Closes #103. Part of #87. For 0.17.6.
Output pipes cleanly, help matches behaviour, and the docs match the code after every other batch.
Findings
F042 (medium): every store- or user-derived string that reaches Rich now goes through
util/markup.literal, which escapes asrich.markup.escapedoes but prints a trailing backslash once, whatever follows it. That covers member names, attribute values, labels and paths inview,lsandsplit's plan, and in the messages, status lines and progress descriptions ofcommands/,conversion/,core/andformats/.pct_counts[mt]andX_[red]umapprint as stored, and[/x]no longer crashes the command.F096:
view FILE ENTRYon a missing entry raises, so the CLI printsError:on stderr and exits 1, aslsdoes. A new_failhelper incli.pyprints every command's error. It prints a KeyError's message without the quotesstr()adds.F151: the stderr console no longer forces a terminal. In a log or a pipe there are no colour, cursor or spinner codes, and a plain message, such as the
Error:line, is not wrapped at 80 columns. On a terminal nothing changes.F160:
convert --helpescapespyadata-cli\[spatial]and\[pickle]. A test renders every command's help and checks that every bracketed word survives.F161:
main(argv)andmain_deprecated(argv)passargvto the app. Givenargv, a failure raisesSystemExitfor the caller to catch. Only the command itself ends the process at once (_leave).F159:
--inplacehelp now says the source is replaced by a store written beside it first (subset, recast, and every import).--to's help and its refusals list only the formatsconvertcan write (conversion.writable_formats()).convert --helpseparates the formats it reads and writes from those it only reads, and no longer claims every check runs before the output exists.--suffix, create's generated names (zero-padded to the axis length), concat's--output, and flag tables forls,create,exportandimport.--include-attrs, andexport sparse --chunk.F149: AGENTS.md and developers.md, kept in step, now cover:
storage/useselements/);infoalias;formats/'s parquet, pickled, pandas_frame and validate modules;_fail;--chunkand--no-space-checkoptions;The pyproject comments on anndata and pyarrow are corrected too.
F150: STATUS.md loses the two released "Fixed in" bullets. It gains waves 3 and 4 as steps, and the follow-ups from every batch's hand-over notes.
F155: testing.md gains a table of the CI jobs, under the names GitHub shows, and rows for the 15 test files missing from its map.
F157: troubleshooting.md and pipelines.md now list what
--chunkdoes not bound: obs and var columns, images (export, import and a Visium conversion's tissue images), a pycisTopic pickle, and--in-memory. The 0.17.1 changelog entry no longer says images are not loaded whole.tests/test_docs_are_accurate.pychecks two new kinds of drift:convertflags may be named on its page underreference/formats/instead.The second check found
create --zarr-format,ls --plain,export's--chunk,--in-memory,--max-elementsand--include-attrs,import dataframe --index-columnandconcat --outputmissing from the reference.Hand-over items
Handled:
convert --help, the convert guide, commands.md andcommands/convert.py's docstring said every check runs before the output exists. They now say the input and options are checked first, and what only the data shows (an index out of range) stops the copy, with staging keeping the output from appearing.export array's help no longer says stdout holds the array in memory (checked: already reworded). B8c'sexport image --scalehelp: checked, accurate.Left, with why:
B5: Python warnings still reach stderr with their source line. Printing them as
Warning: ...changes behaviour beyond this batch's findings. Installed in the app callback, it would also take warnings away from pytest's capture. It is in STATUS.md.CI: the
external-readersjob is named "10x h5 against scanpy and PyTables" andcompatibility"anndata 0.8-0.13", though the first checks every converter and the second reads 0.7.8 too. Renaming a job renames a required check, so that waits for whoever owns the branch rules. testing.md describes what each job really runs.test_concat_versions.pyis anintegrationtest that no CI job runs. Both are in STATUS.md and testing.md.B6b (B6b: Block sizing and Zarr v3 shards #121): a trial merge (
git merge-tree) conflicts in two places:cost guardsjob) edits where this PR puts a table of jobs.Once B6b: Block sizing and Zarr v3 shards #121 lands, two more things are needed:
test_block_layout.pyneeds a row in testing.md's map, which the new docs test enforces;cost guards (py3.12/3.13)job needs a row in the CI tables of testing.md, AGENTS.md and developers.md.Whichever PR merges second does all of this.
Changelog entry
Fixed
[mt]of scanpy'spct_counts[mt]as a style tag.view,ls,splitand messages showedpct_counts, a name the next command could not find, and a name, label or attribute holding[/x]stopped the command. Every name, label, path and value from a store or the command line is escaped now.view FILE ENTRYon a missing entry fails (#103). It printed itsError:line on stdout and exited 0, so a script took the message for the entry's description. It goes to stderr with exit code 1 now, aslsdoes.grep '^Error:' adata.logfound nothing. Off a terminal there are no codes now and an error is one line. On a terminal nothing changes.convert --helpnames the extras to install (#103):pyadata-cli[spatial]andpyadata-cli[pickle]showed aspyadata-cli.--tolists only the formatsconvertcan write;--inplacesays the source is replaced by a store written beside it, not modified directly;convert --helpsays which formats are only read.adata.cli.main(argv)runsargv(#103). It parsed the process's own command line instead.Independent review
A fresh subagent with none of this session's context reviewed the diff against the audit entries and AGENTS.md. It ran about 40 commands with both streams redirected, on stores whose names, labels, attribute values and paths held brackets,
[/x]and trailing backslashes. It found no Rich MarkupError and no escape codes off a terminal, and every error line stayed one line. It also confirmed colour on a pseudo-terminal. Its findings, and what was done:Medium, fixed. split's header put a literal
]after the escaped column (obs\[{column}]), so a columnx[redclosed into a[red]tag. It now escapes the wholeobs[column], and AGENTS.md and developers.md say never to write brackets around a value in a template.Low-medium, fixed.
rich.markup.escapedoubles a trailing backslash, so a name ending in\printed\\wherever text followed it (view, concat's skip warnings,Wrotepaths, import messages). The newutil/markup.literalreplaces everyescapecall. A parametrised test renders it before a tag and before text.Low-medium, fixed.
main(argv)still ended an embedding process withos._exiton failure. It raises now;_leaveapplies only to the command.Low, fixed in the docs. Usage errors (exit 2) are Typer's boxed message, not an
Error:line. pipelines.md and troubleshooting.md now confine the one-line promise to exit code 1. Changing Typer's error rendering would change help rendering too.Low, fixed. testing.md named the wrong jobs as misnamed, and overstated the
rank_genes_groupscoverage.Low, fixed. In AGENTS.md:
workflow lintjob;developers.md now mentions
main(argv),validateandpandas_frameas AGENTS.md does.Low, fixed. The lists of what is held whole now include
--in-memoryin troubleshooting.md, and JSON (import dictparses whole,export dictbuilds whole) in both. pipelines.md's--chunktable gainsexport sparse.Tests
tests/test_cli_output.py(F042, F096, F151, F160, F161, and the review's findings 1 to 3). Each failed before its fix.tests/test_docs_are_accurate.py.tests/test_cli.py::test_view_entry_not_foundexpected exit 0; it now expects exit 1 and the message on stderr.COLUMNS=80 uv run pytest -m "not integration" --cov=adata --cov-fail-under=90: 3902 passed, 7 skipped, 4 xfailed; coverage 95.79% (head 9d488c2).test_write_safety.py::test_export_writes_to_devices_and_pipes_as_they_are. Its FIFO reader stayed blocked inread()with no writer left, and since the reader thread is not a daemon, pytest could not exit. The test passed 15 of 15 runs alone and 3 of 3 with all oftest_write_safety.py, and nothing in this PR touches that path. It is recorded in STATUS.md as a follow-up.🤖 Generated with Claude Code