-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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(cli): CliArguments
interface generated from config.ts
#32556
Conversation
CliArguments
type generated from config.tsCliArguments
type generated from config.ts
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.
/** | ||
* The CLI command name followed by any properties of the command | ||
*/ | ||
readonly _: Array<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.
This is supposed to be typed [Command, ...string]
but I don't have a good way of doing that via code-gen (enlighten me if you think differently). Instead I plan to validate that the first array element is a valid Command when I am converting the input into this type.
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.
we might also want to pull out the additional elements into its own STACKS
property, as that is what the current Arguments
type does:
export type Arguments = {
readonly _: [Command, ...string[]];
readonly exclusively?: boolean;
readonly STACKS?: string[];
readonly lookups?: boolean;
readonly [name: string]: unknown;
};
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.
-
You could just not include it hear, keep this interface to just the auto-generated stuff and inside the CLI have an interface extending the codegen'd one with whatever extra types you need.
-
Misusing
ambient
should work:Type.ambient('[Command, ...string[]]')
You'll have to importCommand
manually though.
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.
Otherwise we would need proper support for this in typewriter
:
- A new
Type.variadic(elementType: Type)
- A new
Type.tuple(...elementTypes: Type[])
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #32556 +/- ##
==========================================
+ Coverage 78.86% 79.13% +0.26%
==========================================
Files 108 107 -1
Lines 7165 7131 -34
Branches 1319 1319
==========================================
- Hits 5651 5643 -8
+ Misses 1330 1304 -26
Partials 184 184
Flags with carried forward coverage won't be shown. Click here to find out more.
|
CliArguments
type generated from config.tsCliArguments
interface generated from config.ts
➡️ PR build request submitted to A maintainer must now check the pipeline and add the |
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
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.
Note: The package rename could have been a separate PR that would be very easy to approve.
Co-authored-by: Momo Kornher <kornherm@amazon.co.uk>
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
This pull request has been removed from the queue for the following reason: Pull request #32556 has been dequeued. The pull request could not be merged. This could be related to an activated branch protection or ruleset rule that prevents us from merging. (detail: 3 of 6 required status checks have not succeeded: 2 expected and 1 failing.). You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. If you want to requeue this pull request, you need to post a comment with the text: |
…nto conroy/codegenwork
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
This pull request has been removed from the queue for the following reason: Pull request #32556 has been dequeued. The pull request could not be merged. This could be related to an activated branch protection or ruleset rule that prevents us from merging. (detail: 4 of 6 required status checks are expected.). You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. If you want to requeue this pull request, you need to post a comment with the text: |
@Mergifyio refresh |
✅ Pull request refreshed |
Comments on closed issues and PRs are hard for our team to see. |
This PR:
CliArguments
which is an interface created from the cli source-of-truth inconfig.ts
.yargs-gen
intocli-args-gen
to better reflect what we are generating nowThe purpose of
CliArguments
is to eventually replace our currentArguments
type, turning it into a strongly-typed object.Arguments
today looks like this:And because the last line in the definition essentially accepts any kind of property, we end up passing in and using values that are not documented anywhere. The purpose of this PR is to introduce a better type to enforce that the
args
object incli.ts
only holds values we expect. We are not currently using the new type anywhere; that will be done in a subsequent PR.The success criteria of this PR is that we are generating a new type from the source of truth that will eventually represent the type of the object we receive from CLI inputs.
Part of #32474
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license