-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Fix GH#18217 issue for FileLog. #27430
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
The only reason this removes the need for a cast is because we don't have I think a simpler fix would be to just add |
I think keeping |
Maybe we could generate the string lazily? |
If done that way, it needs to create unnecessary function expresions. |
I have updated this PR. Now the parameter to |
@@ -13,15 +11,25 @@ namespace ts.server.typingsInstaller { | |||
|
|||
class FileLog implements Log { | |||
private logEnabled = true; | |||
constructor(private readonly logFile?: string) { | |||
private readonly logFile: string = ''; |
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 think it would be better if logEnabled
was just rolled into the optionality of logFile
-- if the log is enabled logFile
should be set, otherwise logFile
will be undefined. Then we don't have to initialize logFile
with a value it should never actually use.
@Andy-MS I updated the PR, didn't see your last comment at first. If I got your last comment correct, you think that we should use Or do you like the current state of my proposed changes? |
Right. |
@Andy-MS Updated PR. |
Fixes occurrence of #18217 in
FileLog
class.