-
Notifications
You must be signed in to change notification settings - Fork 4.3k
chore(cli): use typed errors ToolkitError and AuthenticationError in CLI
#32548
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
Conversation
Signed-off-by: Sumu <[email protected]>
Signed-off-by: Sumu <[email protected]>
Signed-off-by: Sumu <[email protected]>
Signed-off-by: Sumu <[email protected]>
Signed-off-by: Sumu <[email protected]>
Signed-off-by: Sumu <[email protected]>
Signed-off-by: Sumu <[email protected]>
Signed-off-by: Sumu <[email protected]>
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.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.
Signed-off-by: Sumu <[email protected]>
…m/aws/aws-cdk into sumughan/typed-errors-cli-toolkit
Signed-off-by: Sumu <[email protected]>
ToolkitError and AuthenticationError in CLI ToolkitError and AuthenticationError in CLI
Signed-off-by: Sumu <[email protected]>
Signed-off-by: Sumu <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #32548 +/- ##
==========================================
+ Coverage 80.54% 80.64% +0.10%
==========================================
Files 106 107 +1
Lines 6954 6996 +42
Branches 1287 1290 +3
==========================================
+ Hits 5601 5642 +41
Misses 1175 1175
- Partials 178 179 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Signed-off-by: Sumu <[email protected]>
…onError func on the ToolkitError class Signed-off-by: Sumu <[email protected]>
Signed-off-by: Sumu <[email protected]>
Signed-off-by: Sumu <[email protected]>
…m/aws/aws-cdk into sumughan/typed-errors-cli-toolkit
Signed-off-by: Sumu <[email protected]>
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.
Apologies if some of these questions were already asked/responded to by momo.
Also please configure the eslint rule for this package only to enforce we don't accidentally add new untyped errors.
I don't see where you've resolved this request
| /** | ||
| * The type of the error, defaults to "toolkit". | ||
| */ | ||
| public readonly type: string; |
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.
is this being used anywhere other than tests?
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.
Nope (not yet at least) - I am also copying the implementation from ConstructError again here:
| public abstract get type(): string; |
Adding a type property is also the third instruction on Momo's ticket: #32347
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.
again i wish you had a real reason beyond Momo telling you to. but this is fine, i suppose
| */ | ||
| public readonly type: string; | ||
|
|
||
| constructor(message: string, type: string = 'toolkit') { |
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.
should we restrict type to either toolkit or authentication? what was the decision making behind typing it as string?
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.
also, should the constructor be private?
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.
Both ConstructError and ValidationError have public constructors:
| constructor(msg: string, scope: IConstruct) { |
I could make it an enum with string values? But I'm pretty sure Momo wanted a type property that is a string (third bullet point here): #32347
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.
for all of these questions its my hope that you answer them with your reasons for why you've done something, not because Momo said to or because a prior implementation did so. Both of those things can be true, but you should at least know why you're doing it.
now for this case my question was a silly one. you have a public constructor because you are calling new ToolkitError everywhere
|
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.
I've approved. I will leave it to you to ensure that the CLI integ tests pass + to override the codecov failures
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
|
Comments on closed issues and PRs are hard for our team to see. |
Closes #32347
This PR creates two new error types,
ToolkitErrorandAuthenticationErrorand uses them inaws-cdk.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license