Add option to write output to a file and fix minor TypeScript coding style & formatting issues. #49
Conversation
…style & formatting issues.
| if !reference.dependencies.is_empty() { | ||
| println!("{}.addOverride('DependsOn', [", camel_case(&reference.name)); | ||
| append_str_with_linebreak(output, &format!("{}.addOverride('DependsOn', [", camel_case(&reference.name))); |
There was a problem hiding this comment.
For brevity, would line_break be better? Since the first one is output gives away the goal.
There was a problem hiding this comment.
Would append_with_line_break be better? line_break is too short and does not seem to explain what the method does.
There was a problem hiding this comment.
append_with_newline sound good?
| Option::Some(format!("{}", true_expr)) | ||
| } else { | ||
| Option::Some(format!("{} ? {} : {}", bool_expr, true_expr, false_expr)) | ||
| } | ||
| } |
There was a problem hiding this comment.
The problem is that actually changes semantics of the if statement -- and we can't compress to only the true value
As am example:
{ "Fn::If" ["OnDemand", { "ProvisionedWriteThroughput": 10 }, "AWS::NoValue"] }
No Value turns into a false_expr of {} -- which is technically incorrect as well -- as I learned through experimentation 😂. but translating that to only the true expression would actually be an incorrect program and lead to an outage in certain scenarios (imagine instead of swapping to null above, it just always output ProvisionedWriteThroughput).
In this case, I think we should swap false_expr to return undefined. Props would already have a union of undefined in cases where a user can optionally supply values. This preserves the original semantics, and will lead to correct TS programs -- opposed to {} which is an Any type -- which would not satisfy interfaces.
There was a problem hiding this comment.
Also, for the [] -- I think in all cases an empty array would satisfy any type that required an array so we can just keep that one.
There was a problem hiding this comment.
Great points. I will keep [].
I just tested undefined, it works in most cases but not all, for example:
Type 'undefined' is not assignable to type 'IResolvable | PolicyProperty'
I guess this is fine for now. I will put a comment and ask user to manually fix that.
There was a problem hiding this comment.
I honestly think that means the if statement is unreachable (as in if the if statement went on the false path, the cfn template would fail). Sadly, noctilucent can't detect that as it's a semantic error.
|
LMAO I might have done something wrong that caused this PR being closed. |
|
I am so confused now. Why is this PR not updated after I pushed a newer commit - https://github.com/yilong-wang/noctilucent/commits/main. It is supposed to work according to https://stackoverflow.com/questions/9790448/how-to-update-a-pull-request-from-forked-repo.
Really? |
|
Alright, I created a new PR #50. Will close this one. |
Tested using my own CloudFormation template. The input file is so huge that it is painful to copy the output (which is also huge) from command line. So having the option to write the output to a file is nice.
I also fixed some minor TypeScript coding style & formatting issues after inspecting the output using eslint.