-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pipeline viz 3 #3
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to work, however the code is not easy to read. Try using more descriptive names and maybe some comments.
Not sure if the following bug was introduced by this PR order was part of it before, but the type information of composite blocks is not displayed despite being in the source code. For example when running the vis for the recursive-composite-blocks.jv
mode, the information that the CarsExtractor
is an instance of CSVExtractor
is not displayed.
while (children.length === 1) { | ||
if ( | ||
isCompositeBlocktypeDefinition(block.type.ref) && | ||
mermaidOptions.compositeBlocks | ||
) { | ||
compositeBlocks.push(block); | ||
} | ||
assert(children[0] !== undefined); | ||
block = children[0]; | ||
chain.push(block.name); | ||
children = pipelineWrapper.getChildBlocks(block); | ||
} | ||
let blockString = chain.join('-->'); | ||
const composites = compositeBlocks | ||
.map((compositeBlock) => processCompositeBlock(compositeBlock, depth + 1)) | ||
.join('\n'); | ||
|
||
if (children.length > 1) { | ||
children.forEach((child) => { | ||
blockString += `\n${block.name}-->${child.name}`; | ||
}); | ||
const childrenString = children | ||
.map((child) => processBlock(child)) | ||
.join('\n'); | ||
return blockString + '\n' + childrenString; | ||
} | ||
return blockString + '\n' + composites; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you add some comments there? esp. the while (children.length === 1)
is unclear
'end \n'; | ||
const processBlock = (block: BlockDefinition, depth = 0): string => { | ||
const chain: string[] = [block.name]; | ||
const compositeBlocks = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's recommended to define the type explicitly when initializing a variable with an empty array. Just like you did in the row before.
I.e. const compositeBlocks: MyType[] = [];
@@ -41,36 +40,90 @@ function escapeHtml(unsafe: string) { | |||
.replace(/'/g, '''); | |||
} | |||
|
|||
export function createMermaidPipeline( | |||
export function processMermaidLinks( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While the term process
is correct, there is no real value in it. Try using more semantically concise variable names and especially method names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed in our meeting, you can merge it
Merge changes from pipeline_viz into main after integrating changes from base repository.