add notification if tsserver logs are left on for more than 7 days - #124149
Conversation
|
Updated and I think resolves all of the things you mentioned. I left a few todo comments about some things that I am not sure about. Is there an easy way to update values inside of the extension context global state? Is it stored in a json file that I can open and set the value in, or is it just easier to write something and use the vscode api? |
Matt Bierner (mjbvz)
left a comment
Matt Bierner (mjbvz)
left a comment
There was a problem hiding this comment.
Looking good overall, I just have a few minor suggestions
| // TODO: Should the fallback for this be 0, or new Date()? | ||
| // 0 Means every user that has logging enabled currently will be notified. | ||
| // new Date() means that users will not be notified until they have changed their | ||
| // logging setting at least once. |
There was a problem hiding this comment.
This is the last thing I was unsure about.
There was a problem hiding this comment.
Let's use zero to notify existing users who have logging enabled but don't have a value for LogLevelMonitor.logLevelChangedStorageKey
|
Updated with the changes. I just have the one last question, commented inline. |
Matt Bierner (mjbvz)
left a comment
Matt Bierner (mjbvz)
left a comment
There was a problem hiding this comment.
Looks good, just remove the todo comment and I'll merge this!
| // TODO: Should the fallback for this be 0, or new Date()? | ||
| // 0 Means every user that has logging enabled currently will be notified. | ||
| // new Date() means that users will not be notified until they have changed their | ||
| // logging setting at least once. |
There was a problem hiding this comment.
Let's use zero to notify existing users who have logging enabled but don't have a value for LogLevelMonitor.logLevelChangedStorageKey
|
Thanks! Great work on this PR This will be in the next VS Code insiders build and is scheduled to be released with VS Code 1.57 |
3 participants
If the typescript server logs are enabled for more than 7 days, then a notification is displayed with an option to disable.
I can make a few more improvements, but I just wanted to make sure I was making the changes in the appropriate area.
This PR fixes #123493.