Respect configured TypeScript diagnostic locale - #4660
Conversation
Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds support for configured TypeScript diagnostic locales in the native extension.
Changes:
- Resolves unified and legacy locale settings.
- Restarts the language server when locale configuration changes.
Show a summary per file
| File | Description |
|---|---|
_extension/src/client.ts |
Applies configured locale during client initialization. |
_extension/src/extension.ts |
Restarts active sessions after locale changes. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Medium
…ping restarts Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
|
Maybe it's cleaner to just have a |
|
Sure, @copilot do what Jake suggested. |
…override Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
Done. |
|
It didn't understand what I said, I meant the Client in the server, as in project.Client |
|
@copilot sounds like you need to make some adjustments. |
…lly from workspace configuration Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
Done. Here's what changed:
|
|
@copilot This is still wrong. There should be no client side element. There should be a UserPreference that reads the locale, and Project should call back through Client to set a new locale, if present. |
Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
Addressed in e83a1d7. Locale is now parsed as a |
Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
| // Inject the current locale so that push-diagnostic messages are formatted | ||
| // in the user's chosen language, even when ctx is the background context | ||
| // (which does not have a locale set by the LSP dispatch loop). | ||
| ctx = locale.WithLocale(ctx, s.client.GetLocale()) |
There was a problem hiding this comment.
@copilot This is new; do we have an existing bug? Should we have attached the locale somewhere else above? What's the deal?
There was a problem hiding this comment.
Yes, this fixes an existing push-diagnostics bug. Pull diagnostics use request contexts decorated with the current locale, but push diagnostics run from the session background context, which has none. Localization happens here during DiagnosticToLSPPush, and attaching the locale to the background context at session creation would freeze it and miss later preference changes, so reading the current client locale at this conversion boundary is intentional.
There was a problem hiding this comment.
@copilot Perhaps Session should have a func that returns the background context + this that everyone can use instead of accessing backgroundCtx directly? This seems like a very easy mistake to make
There was a problem hiding this comment.
Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
The native extension always used VS Code’s display language for diagnostics, ignoring explicit
js/ts.localeand legacytypescript.localeoverrides.Locale resolution
js/ts.locale, thentypescript.locale.auto.Configuration updates
{ "js/ts.locale": "en" }