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
Provide feedback on malformed JSON of argv.json #151659
Conversation
src/vs/workbench/services/configuration/common/jsonEditingService.ts
Outdated
Show resolved
Hide resolved
src/vs/workbench/services/configuration/common/jsonEditingService.ts
Outdated
Show resolved
Hide resolved
src/vs/workbench/services/configuration/common/jsonEditingService.ts
Outdated
Show resolved
Hide resolved
src/vs/workbench/services/configuration/common/jsonEditingService.ts
Outdated
Show resolved
Hide resolved
src/vs/workbench/contrib/localization/electron-sandbox/localeService.ts
Outdated
Show resolved
Hide resolved
0508f00
to
630b931
Compare
src/vs/workbench/contrib/localization/electron-sandbox/localeService.ts
Outdated
Show resolved
Hide resolved
private async writeLocaleValue(locale: string | undefined): Promise<boolean> { | ||
try { | ||
|
||
const content = await this.textFileService.read(this.environmentService.argvResource, { encoding: 'utf8' }); |
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.
This is more 💄 , but reading this method writeLocalValue
I might be confused at first as to why we read the file. How about moving this new block of code into a private method validateLocaleFile
(or similar) so that the logic becomes clearer as to why we read the file before writing?
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.
addressed this.
} catch (error) { | ||
this.notificationService.notify({ | ||
severity: Severity.Error, | ||
message: localize('argvInvalid', 'Your argv.json file is not valid. Please open it and fix any parse errors so that your display language can be set properly.'), |
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.
Maybe we can make this more user friendly: the fact that we store the locale in a file argv.json
is not so important imho. In the end I would just indicate that the settings file where we store the locale is malformed and needs manual fixing.
I think settings editor has a similar error notification, maybe we can borrow the wording.
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.
riffed off
vscode/src/vs/workbench/services/configuration/common/configurationEditingService.ts
Line 388 in 0656d21
return nls.localize('errorInvalidConfiguration', "Unable to write into user settings. Please open the user settings to correct errors/warnings in it and try again."); |
d90d313
to
4ef61e8
Compare
4ef61e8
to
8d4d2a7
Compare
This PR fixes #151200