Rename razor.trace to razor.server.trace, and use LogLevel values, to match Roslyn (and VS Code conventions) #6770
Conversation
… match Roslyn (and VS Code conventions)
| "%configuration.razor.trace.off%", | ||
| "%configuration.razor.trace.messages%", | ||
| "%configuration.razor.trace.verbose%" | ||
| "Trace", |
There was a problem hiding this comment.
Should we still have enum descriptions?
There was a problem hiding this comment.
Personally, I don't think they add any value.
Fixes #9142 Client side that "consumes" this is dotnet/vscode-csharp#6770
| ], | ||
| "description": "%configuration.razor.trace%", | ||
| "order": 90 |
There was a problem hiding this comment.
not sure what this "order" was needed for.
There was a problem hiding this comment.
this ensures the Razor settings are grouped and ordered together (cc @Cosifne )
| @@ -1751,21 +1751,20 @@ | |||
| "description": "%configuration.razor.languageServer.debug%", | |||
| "order": 90 | |||
| }, | |||
| "razor.trace": { | |||
| "razor.server.trace": { | |||
There was a problem hiding this comment.
why is this still called trace when it is actually log level?
why is it not named razor.server.loglevel?
There was a problem hiding this comment.
Just following the same convention as Roslyn. I believe its a VS Code convention too.
There was a problem hiding this comment.
There was a problem hiding this comment.
Does that actually change anything that vscode controls? It seems like basically another config to me.
There was a problem hiding this comment.
If you specify it exactly as they describe there, this setting will output the full json trace logs (when enabled) to the trace output channel you pass into the vscode LSP client.
Since we added that option though for roslyn, we've changed the set of values you can use, and now we have to manually convert our option values and set the trace setting on the LSP client. We do that here - https://github.com/dotnet/vscode-csharp/blob/main/src/lsptoolshost/roslynLanguageServer.ts#L137
There was a problem hiding this comment.
Interesting. So Roslyn (and with this PR, Razor) have <language>.server.trace, but the doco says <language>.trace.server. So, should we be consistent with Roslyn, or with VS Code?
There was a problem hiding this comment.
I guess it makes sense to be consistent with Roslyn (for me). We use dotnet.server.trace because
- Our configs have a common prefix of
dotnet - Our configs for server related configurations are
dotnet.server.xyz - Its tracing configuration, so
dotnet.server.trace
There was a problem hiding this comment.
Since we added that option though for roslyn, we've changed the set of values you can use, and now we have to manually convert our option values
Yep, this is exactly what this PR is doing too, so I agree it makes sense for the setting name to be consistent if the options are.
|
Ping @dotnet/razor-tooling for reviews |
Fixes dotnet/razor#9142
Razor logging used to have a setting name that didn't match the
<language>.server.traceconvention, used its own log levels, and the server ignored the setting anyway. This fixes all of that.Goes with dotnet/razor#9745 and will need a Razor bump before merging