-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Feature/redact params #2672
Feature/redact params #2672
Conversation
@@ -18,6 +18,10 @@ class TreeNode { | |||
return this.__instance; | |||
} | |||
|
|||
get redact() { | |||
return this.options ? this.options.redact || false : false; |
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.
why not simply say this.options && this.options.redact
?
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.
that might return undefined, which works in this context but here it explicitly returns a boolean
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.
Not really, It returns something or false, and it's also a very confusing statement. And in any case, this.options
is always going to be defined so you could say !!this.options.redact
to always get a boolean. Or just this.options.redact
is also fine.
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.
sure will check and modify it to make it more clear
@@ -204,6 +204,7 @@ class CommandLoader extends BaseCommandLoader { | |||
|
|||
this.validateMethod(parent); | |||
|
|||
const redact = this.module.RedactParams || false; |
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.
what is this doing?
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.
it takes a value from static getter from the command
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.
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.
ah yes, missed that.
Thanks in advance for your contribution. Please follow the below steps in submitting a pull request, as it will help us with reviewing it quicker.
features/my-new-feature
orissue/123-my-bugfix
);fixes #758