Repository navigation
docs: correct the release claims that measurement disproved - #178
Merged
Merged
Conversation
Three things were wrong, and all three were about v1.0.0 rather than about the code. The upgrading note said documents differ from v0.13.0 only where the old output did not render. That is false. Running the v0.13.0 sample generators unchanged against this tree shows the real set, and the widest member of it was working before: every document now ends with a line ending, which is a one line diff in every file every caller regenerates. Front matter titles are quoted now, a block title moved into the front matter, and a class annotation moved inside the class body. The note is a table of the measured differences, with the reason for the trailing line ending given rather than implied. Nothing said whether a builder may be shared between goroutines, and v1.0.0 would have frozen the silence. It cannot be shared: a builder appends to an ordinary slice, and two goroutines calling it race, which the race detector reports. SPEC.md says so, and a test pins the shape the contract points callers at instead, one builder per goroutine. mermaid/er carried a sync.Map, which read as a promise the package does not keep: Relationship appends to the body slice in the same call, so two goroutines racing on it race whatever that field is, which -race confirms. It is a plain map now, and the output is byte for byte identical. The README called the library feature complete. The markdown side is; the mermaid side is not and is not meant to be, because mermaid keeps shipping diagram types and seven of the ones it already ships are not built here. The claim is split in two and says which is which. Refs #133
This comment has been minimized.
This comment has been minimized.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR updates v1.0.0 upgrade and stability guidance, documents generated-output differences and single-goroutine builder use, adds concurrent independent-builder coverage, and replaces Mermaid entity storage with a regular map. ChangesBuilder stability and compatibility
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Line 84: Update the affected-scope text in the changelog row describing quoted
Mermaid front matter titles to say “diagrams with a front-matter title,”
distinguishing it from title-statement diagrams.
In `@mermaid/er/entity.go`:
- Around line 97-105: Update Diagram.NoRelationship to lazily initialize
d.entities before writing, so registration works safely on a zero-value Diagram
without panicking. Preserve the existing entity assignment and fluent return
behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 13628d96-7e09-4244-97cb-4e6c578137d8
📒 Files selected for processing (6)
CHANGELOG.mdREADME.mdSPEC.mdcontract_test.gomermaid/er/entity.gomermaid/er/entity_relationship.go
Replacing the sync.Map with a plain map took something away that the
sync.Map gave for free: er.Diagram{} accepted writes, because a zero
sync.Map does. A nil map does not, it panics, and nothing in this library
panics on how it is called. The map is made on first use now, and a test
pins the zero value so the next person to change that field finds out.
The changelog said front matter quoting affects every diagram with a
title. It affects the ones whose title goes in front matter; the types
writing a title statement were never touched by it.
Owner
Author
|
Both right, and the second one is a regression I introduced rather than a nit. The zero value. The map is made on first use now, and The changelog row. Correct — front matter quoting affects the diagram types whose title goes in front matter, not the ones writing a |
Code Metrics Report
Details | | main (575a31c) | #178 (e4545c4) | +/- |
|---------------------|----------------|----------------|-------|
+ | Coverage | 96.4% | 96.5% | +0.0% |
| Files | 79 | 79 | 0 |
| Lines | 3305 | 3304 | -1 |
| Covered | 3189 | 3189 | 0 |
+ | Test Execution Time | 11s | 7s | -4s |Code coverage of files in pull request scope (93.8% → 95.8%)
Reported by octocov |
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.
Refs #133. Three claims in the release prep did not survive being measured. All three are about v1.0.0 rather than about the code, which is why they matter now rather than later.
1. The upgrading note was wrong
It said documents differ from v0.13.0 "only where the old output did not render". They do not.
I checked out v0.13.0's sample generators unchanged, ran them against this tree, and diffed against v0.13.0's committed output — which separates "the sample's options changed" from "the library's output changed":
title: Checkout→title: "Checkout"<<Interface>>The first one was not broken before and changes every file. It is deliberate (MD047, and appending two documents to one writer), but a caller reading "only where the old output did not render" would not expect a diff in every file they regenerate. The note is now a table of the measured differences.
2. The concurrency contract did not exist
Nothing said whether a builder may be shared between goroutines, and tagging v1.0.0 would have frozen that silence into a promise nobody wrote.
It cannot be shared: every builder appends to an ordinary slice.
SPEC.mdnow says so, with the reason it will not change in v1.x — a lock would slow the one-goroutine case every caller has to fix a case nobody has asked for. A test pins the shape the contract points callers at, one builder per goroutine, and it passes under-race.There is deliberately no test that shares a builder: that test is a data race, and under
-raceit would fail on purpose, which is a promise to break the build rather than a test.3.
mermaid/erimplied a safety it does not haveIt carried a
sync.Mapwhile every other builder used a plain slice, soerread as the one concurrency-safe builder. It is not:Relationshipappends to the body slice in the same call. Confirmed with the race detector — sixteen goroutines callingRelationshipon one builder givesWARNING: DATA RACE.It is a plain map now. Output is byte-for-byte identical — no golden file and no generated sample moves.
4. "Feature complete" was overstated
The markdown side is complete by design. The mermaid side is not, and is not meant to be: mermaid 11.16.1 ships seven diagram types this library does not build (
info,ishikawa,cynefin-beta,railroad-beta,swimlane-beta,venn-beta,wardley-beta), which I confirmed by putting each keyword throughmermaid.parse. The README now says which half is finished and which is not, and the gaps are being filed as issues.Verification
go test ./...andgo test -race .pass;golangci-lint run ./...reports 0 issues.sync.Mapswap is output-neutral.Summary by CodeRabbit