Pipeline viz #4
Pipeline viz #4
Conversation
|
|
||
| blockType(block: BlockDefinition): MermaidBuilder { | ||
| this.mermaidLines.push( | ||
| `${block.name}[${block.name}<br><i>${block.type.ref?.name}</i>]`, |
There was a problem hiding this comment.
Invalid type "string | undefined" of template literal expression
-block.type.ref?.name
+block.type.ref?.name ?? ''|
|
||
| blockTypeWithProperties( | ||
| block: BlockDefinition, | ||
| propertyString: string, |
There was a problem hiding this comment.
You did right with differentiating between blockType and blockTypeWithProperties, however, the propertyString needs to be valid mermaid syntax and thus leaks implementation details of the builder.
Ideally, the parameter is some kind of key-value object.
The proper mermaid formatting of these properties, including making sure of the proper formatting (escapeHtml), is part of your concrete builder, not the caller of the builder (mermaid_utils.ts).
There was a problem hiding this comment.
combine into only one blockType
processes dictionary with properties (depending on mermaid options)
| export class MermaidBuilder { | ||
| protected mermaidLines: string[] = []; | ||
|
|
||
| pipelineHead(pipelineName: string): MermaidBuilder { |
There was a problem hiding this comment.
Minor thing: Consistency is king/queen.
You used BlockDefinition as a parameter in other functions and just a name here. But as I've said this is a very minor thing that you'll probably automatically do when you have a bit more experience.
Using BlockDefinition makes more sense in this case IMO.
There was a problem hiding this comment.
do you mean a PipelineDefinition?
| return this; | ||
| } | ||
|
|
||
| compositeBody(subblocks: string[], depth: number): MermaidBuilder { |
There was a problem hiding this comment.
depth can be an internal variable. It's the builder's job to make the output nice.
Opening a composite then increases the depth, closing decreases it.
A more common name for depth in this context is indent btw :)
| return this; | ||
| } | ||
|
|
||
| classAssign(block: BlockDefinition, className: string): MermaidBuilder { |
There was a problem hiding this comment.
The Builder should do that itself when build is called.
| return this; | ||
| } | ||
|
|
||
| newLine(): MermaidBuilder { |
There was a problem hiding this comment.
This is not the job of a caller of a mermaid builder.
When deciding if a method should be available to a caller, try asking yourself this question:
Does it make sense to do X when building a diagram?
E.g. "Does it make sense to insert a new line when building a diagram?" - Not really, right?
However, it does make sense for a mermaid diagram Builder to produce a pretty output with a new line. Therefore, this method is useful for internal use only.
There was a problem hiding this comment.
ok, I could make it a protected method
But how do I achieve the same behavior? Currently, the caller knows in which cases I want a new line after a section.
There was a problem hiding this comment.
split into different parts of members and connect them with new-line
| return this; | ||
| } | ||
|
|
||
| section(section: string): MermaidBuilder { |
There was a problem hiding this comment.
Ah, I got it when reading mermaid_utils.ts. Don't build too early, just continue with the same builder (i.e. use just one instance of a builder = just one new MermaidBuilder()).
There was a problem hiding this comment.
but that would require passing the builder around, instead of adding the output of a (sub)-builder as a section
There was a problem hiding this comment.
remove sections and write lines directly
|
|
||
| constructor(protected options: MermaidOptions) {} | ||
|
|
||
| escapeHtml(unsafe: string) { |
There was a problem hiding this comment.
(Improvement, not required) Make methods that a caller doesn't need or shouldn't use protected.
| .replace(/'/g, '''); | ||
| } | ||
|
|
||
| diagram(diagramType: string, diagramDirection: string) { |
There was a problem hiding this comment.
(Optional improvement / if you want to flex with your TypeScript Skills) Use literal types.
E.g. diagram(diagramType: string, diagramDirection: 'TD' | 'LR') { (Use the values mermaid understands).
Also, be aware that implementation details are leaked here. However, in this case, I'm fine with that (since these are rather generic instructors that just happen to be mermaid syntax).
introduced the builder pattern