docs: correct the README against what the code actually does - #31
Conversation
Audited every claim and API reference in the README against main, and ran every documented example. Seven problems, two of which meant the documented code did not work. The Quick Start never registered a type, but requireTypeRegistration defaults to true — so ReadObjectsAsync<T>() and CountObjectsAsync<T>() threw "Registration missing for type" for anyone following the docs literally. Verified by running the Quick Start verbatim. Registration is now shown, with a note on the opt-out. MessagePack was listed as a supported serializer. There is no such package; only System.Text.Json and Newtonsoft.Json exist. The querying examples used properties the documented Person does not have — DepartmentId, IsActive, Email, Points, RegistrationDate, FirstName, LastName — so none of them would compile as written. Called out rather than bloating the Quick Start type. The key-column rewrite was described as something you get "under requireTypeRegistration: true", which reads as opt-in when that is the default. Corrected, with the In figure alongside the Equals one. FilterType.NotIn was undocumented, as were the set-membership edges that are easy to get wrong: empty-set semantics, nulls, NotIn excluding null members, and the raw-path overload needing Cast<object>(). The strict-mode divergence guard was undocumented. It throws on a write, so callers can hit it; the silent failed delete it prevents is worth stating. "Use connection pooling for multi-threaded applications" contradicted the very next clause about single-connection serialization, and named nothing real. Replaced with what actually helps — batching — and a note that useConnectionPooling controls the driver pool, not the gate. Also documented COUNT(*) and batch key reads under Performance. Every example in the file was then executed against main: writes, reads, counts, In/NotIn, the grouped-Or form, the raw-path cast, the LINQ surface, and the guard throwing. All pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the repository documentation (primarily the README) to align with current main behavior and to ensure the documented examples compile and run as written.
Changes:
- Corrected/clarified type registration requirements in the Quick Start and querying guidance (including strict-mode behavior).
- Removed unsupported serializer claims and improved accuracy around key-column rewrite behavior and set-membership (
In/NotIn) semantics. - Updated performance guidance to reflect actual runtime behavior (COUNT execution, batching, and the connection gate vs driver pooling).
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| > If you turn registration off, or register with a custom key selector delegate, reach those | ||
| > rows through `ReadObjectAsync` / `ReadObjectsByKeysAsync`, or index the property like any | ||
| > other — both measured 0.0 ms against the 71.6 ms scan. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
README.md:131
- The comment says opting out of type registration means you must “supply keys at every call”, but in loose mode you can still call keyless query APIs (e.g., ReadObjectsAsync/CountObjectsAsync) without providing keys; it’s specifically the by-object overloads (and writes without registration) that need a key / key selector.
// Pass requireTypeRegistration: false to opt out and supply keys at every call.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
README.md:131
- The last sentence is a bit too broad: with requireTypeRegistration: false, not every call needs an explicit key (e.g., ReadObjectsAsync/CountObjectsAsync can still run), but key-derived operations need either registration or overloads that take an explicit key/key selector. Rewording here would prevent readers from thinking they must supply keys for all operations when opting out.
This issue also appears on line 181 of the same file.
// requireTypeRegistration defaults to true, so register each type you store.
// Registration tells Tycho how to find an object's key, and the query methods
// (ReadObjectsAsync, CountObjectsAsync, the LINQ surface) throw without it.
// Pass requireTypeRegistration: false to opt out and supply keys at every call.
README.md:184
- This reads like a keyless type can only be accessed by explicitly supplied keys, but ReadObjectsAsync/CountObjectsAsync still work after registration even when there’s no id mapping. It would be more accurate to call out that the by-object overloads can’t be used without an id mapping, rather than implying the type is otherwise unreachable.
`AddTypeRegistration<T>()` finds the id property by convention: `Id`, then `<TypeName>Id`
(matched case-insensitively, and it must have a public getter). A type with no such property
still registers, but without an id mapping — it can then only be reached by keys you supply at
the call site.
Audited every claim and API reference in the README against
main, then ran every documented example. Seven problems — two of which meant the documented code simply did not work.The documented Quick Start threw
requireTypeRegistrationdefaults totrue, but the Quick Start never registered a type. Running it verbatim:So anyone following the docs got as far as writing and reading one object, then hit an exception on the first query in the "Basic Querying" section. Registration is now shown in the Quick Start, with a note on the
requireTypeRegistration: falseopt-out.MessagePack was listed as supported
The Features list claimed "System.Text.Json, Newtonsoft.Json, and MessagePack". There is no MessagePack package — only
TychoDB.JsonSerializer.SystemTextJsonandTychoDB.JsonSerializer.NewtonsoftJsonexist. Removed.Examples referenced properties
Persondoesn't haveDepartmentId,IsActive,Email,Points,RegistrationDate,FirstName,LastName— none are on the documentedPerson, so none of the querying or LINQ examples would compile as written. Pre-existing for the LINQ section; I made it worse by addingDepartmentIdexamples. Called out in one line rather than bloating the Quick Start type.The key-column rewrite read as opt-in
It was described as something you get "under
requireTypeRegistration: true", which reads as a flag to turn on when it is the default. Corrected, and theInfigure now sits alongside theEqualsone (101.2 ms → 0.2 ms).I got this wrong in my own PR descriptions and summaries too — I repeatedly said strict mode "is not the default." It is. The practical effect is that the rewrite, and the guard below, apply by default to any type registered by id property, which is a wider blast radius than I described at the time.
FilterType.NotInand the set-membership edges were undocumentedAdded
NotIn, plus the four things easy to get wrong: empty-set semantics,nullin the set,NotInexcluding null members the wayNotEqualsdoes, and the raw-path overload needing.Cast<object>().The strict-mode divergence guard was undocumented
It throws on a write, so callers can hit it — and the silent failed delete it prevents (
DeleteObjectAsync(obj)returningfalsewhile the row survives) is worth stating plainly.A self-contradicting performance bullet
"Use connection pooling for multi-threaded applications" was immediately followed by "all database access is serialized onto a single connection" — and named nothing real. Replaced with advice that helps (batching), plus a note that the
useConnectionPoolingparameter controls the driver pool, not the gate. Also documentedCOUNT(*)and batch key reads under Performance.Verification
Every example in the file was executed against
main:Also verified against the code: every method the README names exists with the signature shown (
ReadObjectsWithKeysAsync,CreateIndex,DropIndexAsync,ListIndexes,Cleanup, the blob API,SaveAllAsync, the whole LINQ surface), anddocs/indexing-analysis.mdexists.Docs-only — 296 pass / 4 skipped, unchanged.
🤖 Generated with Claude Code