docs: cover the PDF module where the docs already look for it - #47
Merged
Conversation
The PDF export shipped with its own guide, a homepage card and a README bullet, but the three pages a reader consults *about* the library rather than about the feature never learned it existed: the CLI reference listed every `ksef invoice` subcommand except `pdf`, the architecture page walked every layer except `src/pdf/`, and the test inventory stopped at spec 34 while carrying unit counts from several releases ago. - CLI: `ksef invoice pdf` gets the section its siblings have — what it does offline, the flag table, and the optional peer it needs to render at all. - Architecture: the PDF layer alongside the others, said plainly to be the one layer `KSeFClient` does not reach and the package root does not export, with the entry-point split that follows from `pdfmake` being optional — including what that split means for catching its errors. - Tests: specs 35 and 36 in the suite table, a note on why they are the two that authenticate against nothing and leave a reviewable set of pages behind, the `pdf` row in the unit coverage table, and counts that match what the suite actually runs today. Every flag, default, template name and count here was read off the current code or a run of it, not off the feature guide. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M7sLete4AKPr5oHz2cMFqM
|
Warning Review limit reachedNext included review available in 38 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe documentation adds the PDF rendering layer to the architecture guide, documents the ChangesPDF documentation
Estimated code review effort: 1 (Trivial) | ~3 minutes Merge Risk: ⚪ Minimal · up to This documentation-only change adds PDF coverage to the CLI, architecture, and test guides without changing product behavior; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (3 skipped: 3 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The section has carried everything it is going to carry, so it stops being "Unreleased" and names the day it goes out. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M7sLete4AKPr5oHz2cMFqM
The PDF section told the reader to run `npm i "pdfmake@^0.2.20"`, two screens below the page's own instruction to install the CLI globally. The peer is resolved from the installed package's location, not from the working directory, so that combination installs pdfmake somewhere the CLI never looks and the command keeps reporting a missing peer. Both forms are now given, with the rule that decides between them. Checked by resolving `pdfmake/build/pdfmake.js` from a package placed in a global-style tree: it finds a peer installed beside it, which is what `npm i -g` produces, and a project-local install in some other directory is not on that search path at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M7sLete4AKPr5oHz2cMFqM
The PDF layer table said the document version comes from the namespace the root element is bound to. That is true of a UPO and false of an invoice: an invoice is read from the kodSystemowy attribute on KodFormularza and from WariantFormularza, which must agree with each other, and a contributor going by this line would have looked for a namespace check that is not there. Both mechanisms are now stated separately, matching detectInvoiceVersion and detectUpoVersion in src/pdf/parse.ts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M7sLete4AKPr5oHz2cMFqM
"A template can only name paths the document already carries" described half of what a template sees. The renderer also exposes the resolved options and the flags derived from them, and the shipped layouts lean on both: the header prints a logo it was handed, the totals gate on which reading of P_15 this document supports, the page marks itself OFFLINE when no KSeF number was given. A reader taking the old sentence at face value would think none of that was possible without a code change. The claim that matters — data, not script — is unchanged, and now says what the data covers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M7sLete4AKPr5oHz2cMFqM
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.
The PDF export shipped with its own guide (
docs/pdf-export.md), a homepage card and a README bullet. What it never got was a mention on the three pages a reader consults about the library rather than about the feature — so someone reading the CLI reference, the architecture tour or the test inventory would not learn the module exists.What this closes
docs/cli.md— the Invoices section listed everyksef invoicesubcommand exceptpdf. It now has the same treatment as its siblings: what it does (offline, document kind auto-detected), worked examples, a table of all 16 flags with their defaults, and the one thing that bites — rendering needs the optionalpdfmakepeer, without which the command exits with an install hint instead of writing a file.docs/architecture.md— every layer was walked exceptsrc/pdf/. Added to the Source Layout, plus a section in the shape of the neighbouring ones: a file-by-file table, and the three facts that are not obvious from the tree — it is the only layerKSeFClientdoes not reach and the package root does not export, its templates are data rather than script, and each entry point being bundled on its own is what decides how its errors are caught.docs/tests.md— the suite table stopped at spec 34. Specs 35 and 36 are in, with a note on why they are the two that authenticate against nothing, needyarn buildfirst, and deliberately leave a reviewable set of pages in.pdf-preview/. The unit coverage table gained itspdfrow (29 files), and the counts now match what the suite actually runs — they had been carrying numbers from several releases ago.Notes
Every flag, default, template name and count was read off the current code or a run of it, not off the feature guide.
--templatereally does accept the UPO layouts, which is why all five built-ins are listed — checked by rendering one.Docs only: no source, no tests, no behaviour.
markdownlintclean.Heads-up for whoever merges second:
feat/pdf-browser-readyalso editsarchitecture.mdandtests.md, so one of the two branches will need a rebase over the other.🤖 Generated with Claude Code
https://claude.ai/code/session_01M7sLete4AKPr5oHz2cMFqM
Summary by CodeRabbit
ksef invoice pdfCLI command, supported options, and PDF export examples.