-
Notifications
You must be signed in to change notification settings - Fork 52
feat(cli): auto-detect language for single-language custom templates #819
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
feat(cli): auto-detect language for single-language custom templates #819
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #819 +/- ##
==========================================
+ Coverage 82.49% 83.23% +0.74%
==========================================
Files 71 71
Lines 10333 10396 +63
Branches 1273 1306 +33
==========================================
+ Hits 8524 8653 +129
+ Misses 1767 1704 -63
+ Partials 42 39 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Please add unit tests for these changes. Consider separating the basePath and actualBasePath logic into a separate function, or adding some comments to explain it and its relationship with language—it's not straightforward at a glance.
| const entries = await fs.readdir(basePath, { withFileTypes: true }); | ||
| const languageDirs = entries.filter(entry => | ||
| entry.isDirectory() && | ||
| ['typescript', 'javascript', 'python', 'java', 'csharp', 'fsharp', 'go'].includes(entry.name), |
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.
No need to name these, iterate the keys from languageExtensions earlier in the file.
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.
Addressed this by implementing my suggestion. Moved language extensions to a new interface LanguageInfo.
| typescript: ['.ts', '.js'], | ||
| javascript: ['.js'], | ||
| python: ['.py'], | ||
| java: ['.java'], | ||
| csharp: ['.cs'], | ||
| fsharp: ['.fs'], | ||
| go: ['.go'], |
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 already have languageExtensions in this file... https://github.com/aws/aws-cdk-cli/pull/819/files#diff-8a5cd4c3a707b56e2e21dd1333dd816bcc07a507effa6c3ad91d0d04381a39d6R238-R245
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.
Moved language extensions to new interface LanguageInfo, which is the new, consolidated source of truth.
| const langDirPath = path.join(basePath, langDir); | ||
| const hasValidFiles = await hasLanguageFiles(langDirPath, getLanguageExtensions(langDir)); | ||
|
|
||
| if (hasValidFiles) { | ||
| actualBasePath = path.join(basePath, langDir); |
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.
Two variables here have the same value—as long as hasLanguageFiles is a pure function (which it should be, if it's not, it should be corrected).
langDirPath and actualBasePath.
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.
Addressed this by consolidating the two variables.
| let languages = await getLanguageDirectories(basePath); | ||
|
|
||
| // Auto-detect single language templates | ||
| if (languages.length === 0) { | ||
| const entries = await fs.readdir(basePath, { withFileTypes: true }); |
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 are performing fs.readDir here twice with the same parameters. getLanguageDirectories calls it in the same way. Try to reuse the result instead of calling it again so soon. See
| async function getLanguageDirectories(templatePath: string): Promise<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.
Changed the return signature of getLanguageDirectories to include entries, so that we can reuse it here and speed up the code.
| if (hasValidFiles) { | ||
| actualBasePath = path.join(basePath, langDir); | ||
| languages = [langDir]; |
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.
Looks like a silent failure if hasValidFiles is falsy. Shouldn't we fail or at least log a warning in this case?
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.
Removed dead code.
Upgrades project dependencies. See details in [workflow run]. [Workflow Run]: https://github.com/aws/aws-cdk-cli/actions/runs/17625260603 ------ *Automatically created by projen via the "upgrade" workflow* --------- Signed-off-by: github-actions <[email protected]> Co-authored-by: github-actions <[email protected]>
Upgrades project dependencies. See details in [workflow run]. [Workflow Run]: https://github.com/aws/aws-cdk-cli/actions/runs/17643754247 ------ *Automatically created by projen via the "upgrade" workflow* --------- Signed-off-by: github-actions <[email protected]> Co-authored-by: github-actions <[email protected]>
|
Addressed my own comments and added a lot of unit testing to cover both new and old code. Removed repeated code and consolidated the source of truth for supported CDK languages in a new |
Updated
cdk initcommand to auto-detect language for single-language custom templates that are pulled from filesystem using--from-pathso users do not need to specify--languagein this case.Fixes #
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license