-
Notifications
You must be signed in to change notification settings - Fork 1
Improve intrinsic functions and add json fallback parsing #364
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
964ce8e to
d2fe206
Compare
0432828 to
3785bce
Compare
5f5e43b to
57f5fae
Compare
| extractMatches(text, JsonRef, logicalIds); | ||
| } | ||
| if (getAttIndex !== -1) { | ||
| if (text.includes('"Fn::GetAtt"')) { |
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.
Would using IntrinsicFunction enum make these more maintainable?
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.
A lof of the intrinsic functions are being used here, it was easier to read and follow without using the enums here
| current.children?.length > 0 && | ||
| current.children.some((child) => NodeType.isNodeType(child, YamlNodeTypes.TAG)) | ||
| ) { | ||
| const tagNode = current.children.find((child) => NodeType.isNodeType(child, YamlNodeTypes.TAG)); |
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 iterating and checking node type twice, once in the condition to enter this scope (line 510) and again to assign tagNode.
Can we search once and continue if not found?
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 of these exit early since it just looks for the first child and if we compute this before the if, then the search always runs even if the other conditions might be false. This is only looking for first element so it should be fast enough
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.
Was not suggesting to compute before if but rather remove line 510 from if condition, and exit if no tagNode is found after line 512.
| const parent = current.parent; | ||
| if (!parent) break; | ||
|
|
||
| // Handle YAML tags like !If, !Sub, !Ref, etc. |
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 100 line method is getting longer. It would be good to refactor the new Tag change into a method. We can take care of the rest later.
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 just moving the existing logic from below to be up top, only the comments are extra
Description
This PR improves how the CloudFormation Language Server parses and handles intrinsic functions (like !If, !Sub, !Ref, Fn::GetAtt, etc.) in both YAML and JSON
templates. It also adds a JSON fallback parser for malformed/incomplete documents.
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Key Changes
1. SyntaxTree.ts - Core parsing improvements
Intrinsic function path handling:
New JSON fallback parser:
Minor fix:
2. LogicalIdReferenceFinder.ts - Simplified reference extraction