-
Notifications
You must be signed in to change notification settings - Fork 69
chore(toolkit-lib): fix code registry is disconnected from usage #191
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
chore(toolkit-lib): fix code registry is disconnected from usage #191
Conversation
| const code = `CDK_${category}_${levelIndicator}0000` as IoMessageCode; | ||
| return { | ||
| code, | ||
| description: `Generic ${level} message for CDK_${category}`, |
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.
this wasn't used and only existed to satisfy the interface
a081ae8 to
1f4780a
Compare
1f4780a to
f51ec4a
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #191 +/- ##
==========================================
+ Coverage 84.95% 85.00% +0.04%
==========================================
Files 207 207
Lines 35723 35723
Branches 4612 4601 -11
==========================================
+ Hits 30348 30365 +17
+ Misses 5220 5207 -13
+ Partials 155 151 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
iliapolo
left a comment
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.
Dequeued to address minor comments.
| * An IoHost wrapper that adds the given action to an actionless message before | ||
| * sending the message to the given IoHost | ||
| */ | ||
| export function withAction(ioHost: IIoHost, action: ToolkitAction): ActionAwareIoHost { |
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.
Seems like we already have this function here:
| export function withAction(ioHost: IIoHost, action: ToolkitAction) { |
Can we reuse?
| /** | ||
| * The current action being performed by the CLI. 'none' represents the absence of an action. | ||
| */ | ||
| export type ToolkitAction = |
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.
Unrelated to this PR but it just caught my so putting it out here. This list is slightly different than then one in the CLI. Intentional?
aws-cdk-cli/packages/aws-cdk/lib/toolkit/cli-io-host.ts
Lines 91 to 110 in 01acccf
| export type ToolkitAction = | |
| | 'assembly' | |
| | 'bootstrap' | |
| | 'synth' | |
| | 'list' | |
| | 'diff' | |
| | 'deploy' | |
| | 'rollback' | |
| | 'watch' | |
| | 'destroy' | |
| | 'context' | |
| | 'docs' | |
| | 'doctor' | |
| | 'gc' | |
| | 'import' | |
| | 'metadata' | |
| | 'notices' | |
| | 'init' | |
| | 'migrate' | |
| | 'version'; |
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.
Yes, the CLI has more. To be unified soon.
Previously the declared
leveland payload interfaces were only loosely coupled to actual messages being created. This caused a number of issues with incorrectly documented levels and payload interfaces.Instead of the previous approach, the code registry is now providing a strongly typed "message builder" function specific to every registered code. This function uses generics, so it can correctly ensure at build time that payloads are as expected.
This is the first PR of this series to keep the diff small. Next steps will be to make the code registry available to the CLI package and to remove the old helpers for creating messages.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license