Make all arguments as keyword arguments #81
Conversation
Signed-off-by: Sayali Gaikawad <[email protected]>
gaiksaya
requested review from
peterzhuamazon,
bbarani,
rishabh6788,
zelinh,
jordarlu,
prudhvigodithi and
Divyaasm
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #81 +/- ##
=======================================
Coverage 78.57% 78.57%
=======================================
Files 6 6
Lines 434 434
Branches 130 130
=======================================
Hits 341 341
Misses 93 93 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Sayali Gaikawad <[email protected]>
| @@ -249,30 +249,30 @@ export class OsClusterEntrypoint { | |||
| securityDisabled: security, | |||
| opensearchVersion: distVersion, | |||
| clientNodeCount: clientCount, | |||
| cpuArch, | |||
| cpuArch: cpuArch, | |||
There was a problem hiding this comment.
Does it make any difference?
Since passed argument name and accepting parameter name is same, having parameter1: argument1 is same as parameter1. Or is it different?
There was a problem hiding this comment.
I don't think variable name as any significance in mapping (I might be wrong). This is to keep the args consistent and not worry about naming or order of arguments (positional) since the list is only going to grow from now on. Recommending to choose one for better maintenance and passing between classes.
There was a problem hiding this comment.
You are right! TS is weird. The compiler is resetting in my local too. Found this: https://stackoverflow.com/questions/49798156/typescript-variable-name-as-interface-type
Do you think it is a better idea to have different variable names than the keyword arguments to be passed onto as key value pairs ? If not you can close this PR @rishabh6788
Description
Making args consistent with this change. It was a combination of positional args and keywords args causing confusing. This should help later on when we extend the stack to import the context variables as props too. With all args being keywords, the user need not worry about the order. This is a good practice when the number of arguments are more in number.
Also fixes the typo in readme. Following best practices keep security as enabled for sample command.
Issues Resolved
List any issues this PR will resolve, e.g. Closes [...].
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.