Adding DotNotation Support and Widget Framework to existing cli (#1495)#1496
Adding DotNotation Support and Widget Framework to existing cli (#1495)#1496LeighFinegold merged 5 commits intofinos:mainfrom
Conversation
bf25a57 to
401ebac
Compare
401ebac to
210ccc9
Compare
|
@markscott-ms, @aidanm3341 , @willosborne , @Thels - as discussed yesterday, pushing what have so more can contribute in parallel |
|
@LeighFinegold could you add issues for your known todos so they are more visible for others to assign and pick up? |
rocketstack-matt
left a comment
There was a problem hiding this comment.
I've tested this locally and it is frankly incredibly cool . . .
# Example table
{{table nodes['load-balancer']}}
# Example list
{{list relationships property="unique-id"}}
# Example JSON View
{{json-viewer nodes}}produces
One big issue I came across we should fix was when initially passing the output file as in the same directory I was working in (which happened to be the root of the repo) . . . it deleted everything in the directory without warning 😢
I think this is existing behavior, but I wasn't expecting it because I gave an output file not just a directory. Either way, I think we should at the very least add a warning that the output dir will be cleaned and prompt for confirmation. In the case of passing an output file (as opposed to a directory) I don't think it should clean out the directory at all.
Follow up issues will be raised over the next few days and will mark related items as good first issues for cli developers |
It's existing behaviour to do a clean step at the beginning, but I agree a single file should only delete the file and not the whole directory when using the --template option. Will raise an issue describing the problem Raised #1505 |
| } | ||
|
|
||
| registerDefaultWidgets() { | ||
| const widgets: { widget: CalmWidget<unknown, object, unknown>, folder: string }[] = [ |
There was a problem hiding this comment.
nitpick: have this list in alphabetical order of widget name?
calm-widgets/src/widget-registry.ts
Outdated
| import fs from 'fs'; | ||
| import path from 'path'; | ||
| import Handlebars from 'handlebars'; | ||
| import { CalmWidget} from './types'; |
There was a problem hiding this comment.
nitpick: inconsistent spacing within braces
There was a problem hiding this comment.
This should be handled by some automatic linting we run pre-PR.
This won't be the only case of this in the code-base.
There was a problem hiding this comment.
There was a problem hiding this comment.
I addressed the minor comment all the same on last commit
| @@ -0,0 +1,29 @@ | |||
| {{#if ordered}} | |||
There was a problem hiding this comment.
There is a lot of duplicate code in this file - the only things that change are ol <=> ul. Can/should the ordered if/else be around the opening and closing tags only?
There was a problem hiding this comment.
We could delegate the tag to the viewmodel and just print that. Welcome an issue or PR post merge if we want to address this.
There was a problem hiding this comment.
Given the urgency of needing this in - can you raise an issue for this and we can merge without?
cli/src/cli.e2e.spec.ts
Outdated
| afterAll(() => { | ||
| if (tempDir) { | ||
| fs.rmSync(tempDir, { recursive: true, force: true }); | ||
| //rs.rmSync(tempDir, { recursive: true, force: true }); |
There was a problem hiding this comment.
Is this line of code meant to be commented out?
There was a problem hiding this comment.
It has no impact to the build as ultimately everything is discarded in the end, but this is something that is done to view the outputs when there is an issue in more detail. It occurs on the cli.e2e.spec.ts tests because these are the tests where we on doing a full simulation of using the npm module and therefore harder to debug through problems i.e. this is done so i can debug using the index.js.
Testing approach has been brought up in a few office hours. Will follow up with @willosborne and see if we can raise an epic around testing practices we are looking to adopt and cleanup.
There was a problem hiding this comment.
I addressed the minor comment though on last commit
Deletion of directory is existing behavior and will be addressed in a subsequent PR
|
I think that this is a big PR and @LeighFinegold has put a considerable amount of his own personal time into getting this into the state it's in. I've pulled down the PR and it's behaving, looks like @rocketstack-matt also has done it based on the conversations above. I think we should approve and then refactor/improve as we go on instead of trying to make it perfect from the get-go. @markscott-ms - you good with this too? |
Agreed - I've approved, @markscott-ms if you're happy with he issues being raised post merge lets move ahead. |
|
@Thels I agree with merging this in to allow divide and conquer on anything spotted. Functionally and conceptually this looks great. |

Overview
This PR introduces the foundational Widget Framework and Dot Notation capabilities for CALM templating, putting them structurally in the right place:
calm templateandcalm docifycommandscalm docifycommandRelated to: CALM Docify CLI v1.0.0 MVP #1452
Key Changes
Dot Notation Support (for calm template & calm docify)
nodes['unique-id=="service"'])Widget Framework (for calm docify only)
table,list,json-viewerTemplate Engine Integration
What's Working
Dot Notation (Both Commands)
Widgets (calm docify Only)
Future Enhancements
Widgets to be Enhanced
Dot Notation to be Enhanced
CALM-Specific Widgets (Next Phase)
flow-sequence- Interactive sequence diagramsc4-architecture- System architecture viewsrelated-nodes- Node relationship visualizationKnown TODOs
The following has been identified but should not block this initial PR. This will allow other maintainers to dive in and contribute.
Testing
Migration
calm templateandcalm docifyworkflows(@markscott-ms, @aidanm3341 , @willosborne , @Thels , @rocketstack-matt )