-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Simplify server logger #17271
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
Simplify server logger #17271
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.
The refactor looks solid although perhaps somebody else who is familiar with the server code should also review.
src/server/editorServices.ts
Outdated
info(`Project '${project.getProjectName()}' (${ProjectKind[project.projectKind]}) ${counter}`); | ||
info(project.filesToString()); | ||
info("-----------------------------------------------"); | ||
counter++; |
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.
Can you remove the count updating responsibility from printProjects and replace it with const counter = this.externalProjects.length + this.configuredProjects.length + this.inferredProjects.length
. Then printProjects can be lifted out of this function entirely.
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.
@sandersn, counter
is used on line 948, though its still feasible to do something like:
function printProjects(projects: Project[], counter: number) {
for (const project of projects) {
...
counter++;
}
return counter;
}
And then use it like this:
let counter = 0;
counter = printProjects(this.externalProjects, counter);
counter = printProjects(this.configuredProjects, counter);
printProjects(this.inferredProjects, counter);
src/server/editorServices.ts
Outdated
info(`Project '${project.getProjectName()}' (${ProjectKind[project.projectKind]}) ${counter}`); | ||
info(project.filesToString()); | ||
info("-----------------------------------------------"); | ||
counter++; |
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.
@sandersn, counter
is used on line 948, though its still feasible to do something like:
function printProjects(projects: Project[], counter: number) {
for (const project of projects) {
...
counter++;
}
return counter;
}
And then use it like this:
let counter = 0;
counter = printProjects(this.externalProjects, counter);
counter = printProjects(this.configuredProjects, counter);
printProjects(this.inferredProjects, counter);
@@ -172,22 +170,24 @@ namespace ts.server { | |||
} | |||
|
|||
perftrc(s: string) { | |||
this.msg(s, Msg.Perf); |
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.
Could we instead turn Msg
into a string enum? That would allow us to continue to find all references if necessary.
getLogFileName(): string; | ||
} | ||
|
||
export namespace Msg { |
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 turn this into a string enum?
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 should only be one reference to each kind of message -- one each in perftrc()
, info()
, and err()
.
Odd. It looks like Travis caught an error due to an invalid cast, but Jenkins didn't. |
I think the validity of the cast recently changed, so it might have to do with the TypeScript versions they're using. |
Yeah, I just saw it locally after merging from master. |
Fix is already in. #17685 |
Just have
group
instead of separatestartGroup
andendGroup
.Consolidate all noop test loggers.
Don't expose
msg
.