-
Notifications
You must be signed in to change notification settings - Fork 443
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: Refactor hcl2cdk to read nicer #2826
Conversation
Signed-off-by: hashicorp-copywrite[bot] <110428419+hashicorp-copywrite[bot]@users.noreply.github.com>
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
@@ -22,6 +23,48 @@ const nodeIds = [ | |||
"local.service_name", | |||
]; | |||
|
|||
// Transforms a path with segments into literals describing the path |
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 from expression.ts since we weren't using it anywhere else other than in this test suite
@@ -6,6 +6,9 @@ import { logger } from "@cdktf/commons"; | |||
import { AttributeType } from "@cdktf/provider-generator"; |
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.
Trying to consolidate all type related functions to this file rather than them living in expression.ts
@@ -2,33 +2,24 @@ | |||
// SPDX-License-Identifier: MPL-2.0 |
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.
Useful to look at this file commit by commit. First commit basically takes out all the functions that aren't related to TF Expr -> TS, and then the subsequent commits clean up the actual conversion
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
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.
Took a bit to go through the commits as Githubs diff view was making changes hard to track, but I really like this clean up 👏
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
I'm going to lock this pull request because it has been closed for 30 days. This helps our maintainers find and focus on the active issues. If you've found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Related issue
Fixes #2821
Fixes #2823
Description
During the development of 0.16, we deliberately didn't spend a lot of time to refactor and clean up the monolith that
expression.ts
was becoming. Now that the release is out, we can start cleaning it up.This also removes the use of async when converting from the terraform expression AST to TypeScript. That was added in a hurry, but was not really required. This simplifies the code somewhat.
Checklist