Implement diagram output - #18
Conversation
📝 WalkthroughWalkthroughAdds Mermaid ER diagram generation and rendering, enriches documented symbols with member declarations and a name accessor, reorganizes glossary into a dedicated package, adjusts classpath handling to include JDK base modules, and adds file output utilities and website Mermaid support. Changes
Sequence DiagramsequenceDiagram
actor User
participant Collector
participant Documentation
participant Diagram
participant EntityModel
participant Writer
participant Filesystem
User->>Collector: collectSymbols(packageName)
Collector->>Documentation: build DocumentedSymbols (incl. declarations)
Documentation-->>Diagram: pass Documentation
Diagram->>Documentation: filter class symbols
Diagram->>EntityModel: buildDocEntity(symbol)
EntityModel->>EntityModel: extract fields & term types
EntityModel->>EntityModel: infer associations (Option/IterableOnce)
EntityModel-->>Diagram: return Entity objects
Diagram->>Diagram: asMarkdown(direction)
Diagram-->>Writer: Markdown string
Writer->>Filesystem: write(path, contents)
Filesystem-->>User: file persisted
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
|
I only skimmed through the changes but from this the following questions/comments arose:
|
|
|
I tried this approach and it makes much more sense now. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@domainDocs4s-core/src/main/scala/domaindocs4s/collector/TastyContext.scala`:
- Around line 23-29: The catch block in the val jdkBase (inside TastyContext)
currently catches Throwable and should be narrowed to avoid swallowing fatal
errors; change the catch to handle Exception or preferably
scala.util.control.NonFatal (e.g., case NonFatal(_) => Nil) around the
FileSystems.getFileSystem(URI.create("jrt:/")) / jrtFs.getPath("modules",
"java.base") call, and add the necessary import for scala.util.control.NonFatal
if you choose that approach.
In `@domainDocs4s-core/src/main/scala/domaindocs4s/output/diagram/Diagram.scala`:
- Around line 104-108: Diagram.write duplicates Writer.apply and introduces an
extra println side-effect; remove or consolidate it by deleting Diagram.write
and updating any callers to use Writer.apply (or have Diagram delegate to
Writer.apply) and ensure the println is removed so file writes go through the
single Writer.apply implementation (refer to Diagram.write and Writer.apply to
locate the code to change).
- Around line 51-60: buildAssociations currently collects (s.name,
determineRelationshipType(...)) pairs then calls .toMap which silently drops
duplicate keys when multiple fields reference the same documented entity; update
buildAssociations to preserve all associations by returning a
grouped/multi-value structure instead of a Map that loses duplicates—e.g.,
change the return to Map[String, List[Association]] (or List[(String,
Association)]/MultiMap) by grouping the flatMap results by s.name and merging
with a chosen strategy (e.g., collect all Association entries or reduce to the
most permissive cardinality); adjust the callers of buildAssociations
accordingly and use the existing helpers isFieldRelatedToSymbol and
determineRelationshipType to build the list before grouping.
- Around line 70-77: isTypeArgument only checks the top-level applied type args
and misses nested generics (e.g. Option[List[Foo]]), so modify isTypeArgument to
recursively inspect argument types: for each arg in the AppliedType (at.args) if
it's a TypeRef check tr.optSymbol.contains(symbol), and if it's an AppliedType
recurse into its args (or generally apply the same logic to nested
TypeRefs/AppliedTypes); ensure the function still accepts TermSymbol and Symbol
and uses typeOfTerm(field) as the entry point, preserving existing pattern
matches for AppliedType and TypeRef while adding the recursive descent to find
deeply nested occurrences.
In `@domainDocs4s-core/src/main/scala/domaindocs4s/output/diagram/Entity.scala`:
- Around line 28-29: The function escapeMermaidSpecialChars currently strips
dollar signs which can corrupt Scala compiler-generated names; update
escapeMermaidSpecialChars to stop removing "$" and instead replace it with a
readable character (e.g., "." or "_") while keeping the existing quote-escaping
logic (the method name to change is escapeMermaidSpecialChars); ensure the
replacement is applied via text.replace("$", ".") (or "_") so Scala
inner-class/companion names remain distinguishable in diagrams and document the
choice in a comment near the function.
- Around line 3-7: The Entity case class uses Map for fields and associations
which is non-deterministic for sizes >4; change Entity to use an ordered
collection (e.g., ListMap[String,String] and ListMap[String,Association] or
Vector[(String,String)] / Vector[(String,Association)]) so iteration order is
stable, update all call sites that construct Entity (notably Diagram.buildFields
and Diagram.buildAssociations) to produce ListMap.from(...) (or the chosen
ordered type) and ensure Entity.asMarkdown still iterates over those ordered
collections to produce deterministic Mermaid output and stable snapshots.
In `@domainDocs4s-core/src/main/scala/domaindocs4s/output/Writer.scala`:
- Around line 1-12: The project has duplicate file-write logic: update
Glossary.write to delegate to the shared Writer.apply instead of duplicating
Files.write (the same change should be considered for Diagram.write too);
enhance Writer.apply to explicitly use a charset (e.g., UTF-8) and ensure parent
directories exist before writing (create parent dir via Path.of(path).getParent
and Files.createDirectories if non-null) so consumers like Glossary.write and
Main.scala use a single, robust I/O primitive.
In `@domainDocs4s-examples/src/test/resources/banking/diagram.md`:
- Around line 1-40: The generated Markdown from Diagram.asMarkdown currently
preserves a leading newline due to using stripMargin without trimming; update
the asMarkdown implementation to trim the resulting string (e.g., call .trim or
equivalent on the output of stripMargin) before returning so the leading blank
line is removed while keeping the rest of the Mermaid content intact; reference
Diagram.asMarkdown and the place where stripMargin is used to locate the change.
In `@website/package.json`:
- Line 29: The devDependency "@docusaurus/theme-mermaid" is versioned at ^3.9.2
while other `@docusaurus/`* packages use ^3.7.0; update the package.json entry for
"@docusaurus/theme-mermaid" to ^3.7.0 (or alternatively update all other
`@docusaurus/`* entries to ^3.9.2) so all Docusaurus packages share the same
version; after changing the version string for "@docusaurus/theme-mermaid" run
npm/yarn install and verify the lockfile is updated and the site builds
successfully.
🧹 Nitpick comments (10)
domainDocs4s-core/src/main/scala/domaindocs4s/output/diagram/Association.scala (1)
3-7: Consider addingOneOrMorecardinality.Standard ER notation includes four cardinalities:
ExactlyOne(1..1),ZeroOrOne(0..1),ZeroOrMore(0..), andOneOrMore(1..). The last one is missing here. If this is intentional for the initial scope, a brief comment noting the omission would help future contributors.Suggested addition
enum Association { case ExactlyOne case ZeroOrOne case ZeroOrMore + case OneOrMore }domainDocs4s-core/src/main/scala/domaindocs4s/output/Writer.scala (1)
8-9: Specify charset explicitly ingetBytes.
docs.getBytesuses the platform-default charset, which can vary across environments and produce inconsistent output. Preferdocs.getBytes(StandardCharsets.UTF_8).Proposed fix
-import java.nio.file.{Files, Path} +import java.nio.file.{Files, Path} +import java.nio.charset.StandardCharsets object Writer { def apply(docs: String, path: String): Unit = { - val _ = Files.write(Path.of(path), docs.getBytes) + val _ = Files.write(Path.of(path), docs.getBytes(StandardCharsets.UTF_8)) }domainDocs4s-core/src/main/scala/domaindocs4s/collector/Collector.scala (1)
11-19: Good addition of thenameaccessor — but Glossary.build still inlines the same logic.
DocumentedSymbol.namecentralizes name resolution, butGlossary.build(Glossary.scala lines 73 and 82) still uses the manuals.nameOverride.getOrElse(s.symbol.name.toString)pattern. Consider updatingGlossary.buildto call.nameinstead to keep things DRY.domainDocs4s-examples/src/main/scala/domaindocs4s/order/Main.scala (1)
24-25: Consider usingWriterinstead ofGlossary.write.Line 25 uses
Glossary.write— ifGlossary.writeis deprecated in favor of the newWriterutility, this example should be updated to demonstrate the preferred API:Writer(glossary, "glossary.md").domainDocs4s-core/src/main/scala/domaindocs4s/output/diagram/Direction.scala (1)
3-9:Direction.Noneshadowsscala.None, preferOption[Direction]instead.The
Nonecase shadowsscala.None, which can cause confusion in files that work with bothOptionandDirection. SinceNoneacts as a sentinel for "no direction specified," usingOption[Direction]as the parameter type inDiagram.asMarkdownis more idiomatic and eliminates the need for this case entirely.♻️ Suggested change
enum Direction { case TB case BT case LR case RL - case None }Then in
Diagram.asMarkdown:def asMarkdown(direction: Option[Direction] = None): String = { // ... val directionLine = direction.map(d => s" direction $d\n").getOrElse("") // ... }domainDocs4s-examples/src/test/scala/domaindocs4s/banking/BankingTest.scala (1)
34-37: Test uses default direction while the Main example usesDirection.TB.The test calls
asMarkdown()with the defaultDirection.None, butMain.scalademonstratesDirection.TB. Consider adding a test case with an explicit direction to cover that rendering path and catch regressions in the direction line output.domainDocs4s-core/src/main/scala/domaindocs4s/output/diagram/Diagram.scala (2)
89-94:typeOfTermthrows hard exceptions on unexpected tree shapes — fragile for a documentation tool.A single symbol with an unexpected or missing tree will crash the entire diagram build. For a documentation generator, it's better to be resilient and skip problematic symbols with a warning rather than abort.
♻️ Suggested approach
- private def typeOfTerm(t: TermSymbol): Type = t.tree match { - case Some(v: ValDef) => v.tpt.toType - case Some(d: DefDef) => d.resultTpt.toType - case Some(other) => throw new Exception(s"Unexpected tree type for $t: ${other.getClass.getSimpleName}") - case None => throw new Exception(s"No tree found for term symbol $t") - } + private def typeOfTerm(t: TermSymbol): Option[Type] = t.tree match { + case Some(v: ValDef) => Some(v.tpt.toType) + case Some(d: DefDef) => Some(d.resultTpt.toType) + case _ => scala.None // skip symbols with unexpected/missing trees + }Then update callers to handle
Option[Type]withflatMap/collect.
79-87:ctx.findTopLevelClassis called for every field — consider hoisting the lookups.
optionClsanditerableOnceClsare resolved on every call todetermineRelationshipType. While likely inexpensive for small domains, these are constants that could be resolved once and passed through or cached.domainDocs4s-examples/src/main/scala/domaindocs4s/banking/Main.scala (1)
3-3: Replace deprecatedApptrait with Scala 3's@mainentry point.
Appis deprecated in Scala 3.8.0+ because it relied onDelayedInit, which Scala 3 dropped. For Scala 3–only code, use@main definstead—it's the idiomatic entry point and Scala 3 generates the required JVM main method automatically.♻️ Suggested change
-object Main extends App { +@main def main(): Unit = {website/docs/outputs/diagram.mdx (1)
73-75: The import path resolves correctly, but consider moving generated output to the website directory structure.The relative path
'../../../domainDocs4s-examples/src/test/resources/banking/diagram.md'successfully resolves in Docusaurus and the import functions as intended. However, this pattern of importing from outside the website directory is unconventional. For better maintainability and to follow standard Docusaurus conventions, move the generateddiagram.mdoutput to the website'sstatic/ordocs/outputs/directory instead, so imports remain within the website project structure.
2 participants
#8
I started working on the diagram output. The work is still in progress, but in the meantime I’d be happy to get feedback on whether this is the right direction. (cc @Krever)
Since the code is still far from final, I’ve provided simplified documentation for the new features to make it easier to get familiar with the changes (see
/docs/outputs/diagramand/docs/outputs/custom).While designing the solution (or rather experimenting with it), my goal was to make it easy for users to implement their own diagrams with minimal effort. This influenced several design decisions, such as using
traitsandcase classes(instead ofenums) to modelRelations.There is still one important piece missing. At the moment, relations are always defined against the parent, which is why some of the examples use overcomplicated structures. It should be possible to define the target of a relation explicitly (i.e. another documented symbol).
Summary by CodeRabbit
New Features
Documentation
Improvements