-
Notifications
You must be signed in to change notification settings - Fork 17
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
refactor: Turn on noUnusedLocals, fix errors. #196
Conversation
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.
thanks for doing this, I agree with the spirit of it. just a few questions about the couple of impacted variables.
|
||
/** | ||
* Creates an instance of SimpleCacheClient. | ||
*/ | ||
constructor(props: SimpleCacheClientProps) { | ||
initializeMomentoLogging(props.configuration.getLoggerOptions()); | ||
this.logger = getLogger(this); |
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.
Instead of removing this can you just add a debug log message to the constructor or something? We will be coming back in here and adding logging at some point and I want to make sure it's really easy for devs to figure out how to do it.
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.
I went to do that, and there's very little to log in SimpleCacheClient. All the work is in CacheClient and ControlClient. I think this is at the wrong level.
@@ -55,7 +55,6 @@ export abstract class SdkError extends Error { | |||
protected readonly _errorCode: MomentoErrorCode; | |||
protected readonly _messageWrapper: string; | |||
private readonly _transportDetails: MomentoErrorTransportDetails; | |||
private readonly _stack: string | undefined; |
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.
so we are never populating this anywhere?
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.
eslint and a search for _stack
say we're not.
I know we discussed storing the stack in error messages, but we're not consistent about that.
For some reason, the linter was configured to allow unused local properties. The check is off on nearly all our Typescript repos. There doesn't seem to be a reason not to turn the check on.
Closes #116