Wire up every NfDocsConfig option - #15
Merged
Merged
Conversation
NfDocsConfig advertised seven options through get_example_config(), but only ignore_config_prefixes did anything. Most were aspirational from the start. strip_readme_badges was not: it was wired when the config module landed, then dropped by 65aa08f ("Try fixing the HTML styling regression"), which reverted extractor.py to a pre-config state while chasing a CSS bug. The same commit removed the `nf-docs config` command and undid an unrelated exception-narrowing change. 2fb2a9b restored the ignore_config_prefixes filter but not the rest. - ignore_input_prefixes and include_hidden_params now filter schema inputs. Parameters dropped this way are also suppressed from the Configuration section, rather than reappearing under a different heading. - strip_readme_badges is consulted again. - max_readme_length truncates README content on a line boundary, before images are embedded so the limit measures prose rather than data URIs. - exclude_patterns is appended to the language server's exclusions rather than replacing them; the built-in list must stay non-empty or the server never initialises the workspace. - default_format is read by the CLI when -f/--format is not given, with a warning and an html fallback for an unknown value. - Restored the `nf-docs config` command, without which get_example_config() is unreachable from the shipped tool. include_hidden_params defaults to true rather than the documented false. Hidden parameters have always been shown, so honouring the field as written would have deleted them from every existing user's output. These options only stay honoured because the cache is keyed on the config, which the parent commit added; NfDocsConfig.cache_key() hashes to_dict(), so the newly-live fields are covered without further change. Verified by changing an option against a warm cache. Each option gets a test that fails if it stops being consulted; the existing tests only round-trip values through the dataclass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012iDGBm7Vt6rZ5FsZd1oZux
Quality pass over 9e7397a. No behaviour change: the no-op comparison against the base branch is still byte-identical apart from the timestamp. - Drop SUPPORTED_FORMATS. It answered "is this renderable?" from SINGLE_FILE_OUTPUT_POLICY, but the authority is RENDERERS in renderers/__init__.py, and get_renderer() already raises for unknown formats. output.py imports nothing from the package, so it structurally could not consult the registry - a format in one table and not the other would have passed the guard and then raised anyway. - Derive the --format choices from RENDERERS + FORMAT_ALIASES rather than a hand-written literal, so adding a format is one edit fewer. - Use NfDocsConfig().default_format for the fallback instead of importing DEFAULT_CONFIG to say "html". - Print `nf-docs config` settings with yaml.safe_dump. The isinstance chain reproduced YAML's own bool and empty-list spellings by hand, and sent anything else through repr - not pasteable into config.yaml, which is the whole point of that output. - Move the config-param filter rationale onto the code it describes; the comment there still claimed the criterion was "already in inputs", which this work had made untrue. - build_exclude_list uses dict.fromkeys for the order-preserving dedupe. - LSPClient stores the merged list as file_excludes: the attribute was named after the constructor parameter but did not hold it. - Tests: assert the truncation boundary exactly rather than by line membership, check badge stripping preserves prose on the stripped side too, and share one config-writing helper instead of three copies. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012iDGBm7Vt6rZ5FsZd1oZux
Wiring up the six dead NfDocsConfig options meant a value of the wrong type in ~/.config/nf-docs/config.yaml now reaches real code. Reproduced against the previous commit: - `default_format: 3` — normalize_format() called .lower() on an int, an uncaught AttributeError and a traceback, from inside the guard that exists to keep a bad config from producing one. - `max_readme_length: "lots"` — TypeError comparing str to int, swallowed by the broad except around README parsing, so the README silently vanished from the output. - `exclude_patterns: "tests"` — a bare string where a list belongs, which the example file makes an easy slip. It iterated as characters and sent ['.git', '.nf-test', 'work', 't', 'e', 's'] to the language server. No error, just wrong indexing. NfDocsConfig.from_dict() now checks each value against the type its option expects and falls back to the default with a warning. Doing it there rather than at each use site covers the library path too — nf_docs.extract( config=load_config()) had no guard at all — and means the next option to be added is validated by construction. from_dict also copies its list defaults now; it previously handed out the DEFAULT_CONFIG lists themselves, so mutating a config mutated the defaults for every later caller. Separately, cache_key() no longer hashes default_format. That field only picks the CLI's output format, so including it evicted every cached pipeline — each one a full Language Server run — to change a presentation default. The exclusion is a named set with a test asserting every remaining option does affect the key, so it can't silently drift. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012iDGBm7Vt6rZ5FsZd1oZux
Review findings, all reproduced before and after. The example config was a trap. Uncommenting the suggested `exclude_patterns` entries under `exclude_patterns: []` produced invalid YAML, and a broken file is not a partial failure - load_config() discards the whole thing and silently reverts every option to its default. Someone following the file's own suggestion would lose all their other settings with no error. It is now a fully commented block that uncomments cleanly, with a test that walks every comment block in the shipped example and fails if one uncomments to an orphan list. That test caught a second version of the same bug in my first fix. `default_format: "[/]"` crashed `nf-docs generate` with a MarkupError: the warning path interpolated the user's value into a rich markup string. It is escaped now. The whole point of that branch is to keep a bad config from producing a traceback. `nf-docs config --path` went through console.print, so rich hard-wrapped a long path across two lines when piped, breaking `$(nf-docs config --path)`. `nf-docs config` on an unparseable file printed "(file exists)" followed by the built-in defaults, with the parse warning escaping to stderr unformatted. It now sets up logging first, so the warning appears between the two, and the heading reads "Settings in effect" rather than "Current settings". A list containing one non-string reported "should be a list of strings, got list", which tells the user nothing. It now names the offending element type. max_readme_length is documented as capping README *source text*: the cut happens before local images are embedded, so the stored content can still be large. Previously the docs promised a character cap with no caveat. Also: docs/running.md still said --format always defaults to html and never mentioned the config file or the `nf-docs config` command; both fixed. Added an upgrade note to the changelog, since six options taking effect for the first time can change output for anyone who already has a config file. Finally, DEFAULT_CONFIG and _FIELD_TYPES are hand-maintained in parallel and from_dict indexes both, so a mismatch would be a KeyError at load time - there is now a test asserting they agree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012iDGBm7Vt6rZ5FsZd1oZux
The comment-block grouping used a None sentinel inside a list[list[str]] and a type: ignore to get away with it. itertools.groupby over "does this line start with #" says the same thing without either. Confirmed it still fails against the original broken example config. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012iDGBm7Vt6rZ5FsZd1oZux
Review of the whole PR. The substantive finding: `nf-docs config` printed `default_format: nonsense` under "Settings in effect" while `generate` warned and used html. The command this PR added to tell users what is in effect was reporting something that wasn't. The cause was validating that one option in the CLI while the other six are validated in from_dict. The stated reason - config.py can't import the renderers - didn't hold: output.py is a leaf module that imports nothing from the package and already owns the format tables. It now exposes is_supported_format()/supported_formats(), config.py checks default_format's domain alongside every other option, and _resolve_output_format collapses to one expression at the call site. One diagnostic channel now serves all seven options, and the rich escape() dance goes away with it. That leaves output.py and renderers/__init__.py as two tables that must agree, so tests/test_renderers.py now pins them together. Other findings, all three reviewers converging on the first: - _FIELD_TYPES hand-listed seven options whose types are exactly type(DEFAULT_CONFIG[key]). It's derived now, which deletes the table and the test that existed only to police it against drift. - _describe_expected built three sentence shapes across seventeen lines and never showed the offending value, which is the thing a user needs to spot the typo. Inlined; the warning now prints it. - _has_expected_shape had four branches where two were plain isinstance, with "list of strings" as an implicit fallthrough. Stated explicitly. - from_dict tested `key in data` twice to distinguish absent from invalid. - click.Choice re-derived the format set that renderers/__init__.py already computes for its error message; both call supported_formats() now, and AGENTS.md no longer tells contributors to update the CLI's choices by hand. - Tests: the warning assertion was a copy of one parametrize case, so it's folded in and now covers all eight; example-config tests moved next to their siblings in TestGetExampleConfig; dropped an assertion subsumed by the one below it. Added a test that `config` and `generate` agree. - docs/python-api.md carried a second copy of the option table; it links to the one in running.md and keeps only the two API-specific facts. Skipped, and worth their own change: caching the unfiltered Pipeline and applying config as a view (five of the six cached options are pure post-processing, so toggling one currently forces a full LSP re-run), moving README parsing into readme_parser.py, and generating get_example_config() from field metadata. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012iDGBm7Vt6rZ5FsZd1oZux
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
2 participants
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.
NfDocsConfigadvertises seven options throughget_example_config(). Onlyignore_config_prefixesdid anything. You could copy the example config, set an option, and get silence.All seven now take effect. Output under a default config is unchanged — verified byte-for-byte, see below.
Why they were dead
Not all of them were aspirational.
strip_readme_badgeswas wired when the config module landed incb85a44, and an hour later65aa08f("Try fixing the HTML styling regression") revertedextractor.pyto a pre-config state while chasing a CSS bug. That commit dropped bothget_config()call sites, undid the exception-narrowing from6c2bb9650 minutes earlier, and deleted thenf-docs configcommand fromcli.py.2fb2a9brestored theignore_config_prefixesfilter two hours later; the rest went unnoticed.So
get_example_config()has had no caller in the shipped tool since January — it was reachable only from tests. Theconfigcommand is restored here (--path,--show-example,--init), adapted to theload_config()injection from #14.Per option
ignore_config_prefixesignore_input_prefixesshould_ignore_input_param(), which had no caller.include_hidden_paramstrue— see below.strip_readme_badgesmax_readme_lengthexclude_patternsdefault_format-f/--formatis absent; unknown value warns and falls back to html.Three things worth calling out:
include_hidden_paramsdefaults totrue, not the documentedfalse. Hidden params have always been shown, so honouring the field as written would have deleted them from every existing user's output on upgrade. Anyone who setsfalsegets the filtering the option has always promised.exclude_patternsis appended, not substituted. The hardcoded[".git", ".nf-test", "work"]exists because the language server only initialises a workspace when the configuration differs from its defaults — an empty list means no indexing at all. Substituting the config value would have broken indexing for everyone who hasn't set one.A parameter dropped by
include_hidden_paramsorignore_input_prefixesdoesn't reappear under Configuration. Config params are deduped against the input list, so filtering inputs first would have let a hidden param resurface under a different heading. The dedupe uses the unfiltered schema names.Config values are now validated
Wiring six dead options means a wrong-typed value in
config.yamlreaches real code. Reproduced against the pre-fix commit:default_format: 3—normalize_format()called.lower()on an int. UncaughtAttributeError, from inside the guard that exists to keep a bad config from producing a traceback.max_readme_length: "lots"—TypeErrorcomparing str to int, swallowed by the broadexceptaround README parsing. The README silently vanished from the output with no indication why.exclude_patterns: "tests"— a bare string where a list belongs, an easy slip given the example file. It iterated as characters and sent['.git', '.nf-test', 'work', 't', 'e', 's']to the language server. No error, just wrong indexing.NfDocsConfig.from_dict()now checks each value against its option's expected type and falls back to the default with a warning. Doing it at the boundary rather than at each use site covers the library path too —nf_docs.extract(config=load_config())had no guard at all — and means the next option added is validated by construction.Rewriting that function also fixed an aliasing bug it had: it handed out the
DEFAULT_CONFIGlist objects themselves, so mutating a config mutated the defaults for every later caller.Cache
cache_key()no longer hashesdefault_format. That field only picks the CLI's output format, so including it evicted every cached pipeline — each one a full Language Server run — to change a presentation default. The exclusion is a named set, with a test asserting every remaining option does change the key, so it can't silently drift.Tests
tests/test_config.pyonly round-tripped values through the dataclass, which is why a green suite never caught any of this. Every option now has a test that fails if it stops being consulted — each asserts both settings, so it fails whether an option is ignored or applied backwards.365 passed, 2 skipped.
ruff check,ruff format --checkandty check src/clean.Verification
The no-op guarantee is checked, not asserted: extracting the same pipeline on
mainand on this branch with a default config produces byte-identical JSON apart fromgenerated_at. Each option was then confirmed to visibly change output, and the cache confirmed to re-extract when an option changes rather than serving the previous result.Not verified:
exclude_patternsagainst a live language server. GitHub is blocked by this sandbox's proxy, so the JAR could not be downloaded. The pass-through is tested with a mockedLSPClientand the merge logic is unit-tested, but nobody has watched the real server honour a user-supplied exclusion. Because I couldn't establish whether it treats entries as globs or path prefixes, I changed the example config's"tests/**/*.nf"to plain directory names rather than ship a promise I can't back. Worth a real run before merging.🤖 Generated with Claude Code
https://claude.ai/code/session_012iDGBm7Vt6rZ5FsZd1oZux
Generated by Claude Code