-
Notifications
You must be signed in to change notification settings - Fork 8.6k
Spec: Introduce a mini-specification for localized resource use from JSON #5280
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
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.
Largely I'm okay with this since I already basically spec'd this in #2193, but let's make sure these discussions run their course
`wt -p profile`. I have no answer for how best to resolve this, except "get | ||
used to it". | ||
|
||
Since we also emit the name `cmd` in the user's settings template, we will want |
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.
Yikes I think a comment would probably be best here, I don't think we want to have to burden users with using the longer form in userDefaults
. Esp. considering there's only 2 profiles in there by default, if they copy-pasta the cmd one, that might get really confusing.
|
||
```json | ||
{ | ||
"name": { "resourceKey": "CommandPromptDisplayName" } |
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.
Do we want resourceKey
or just key
?
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.
+1 to key as it’s more concise
|
||
As above in Compatibility. | ||
|
||
We need a fallback in case a resource cannot be found. |
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.
"name": { "key": "CmdDisplayName", "default": "cmd" }
maybe?
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.
Not sure about having a default. Things we specify should actually assert during DEBUG that they don’t exist, because it’s very important that we land all the resource keys right. Things the user specifies that don’t exist should return something like [KEY]
so they can see the error of their ways.
Either way, I’ll include it in the spec
|
||
## Future considerations | ||
|
||
If we allow the command palette's contents to be driven through JSON |
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.
oh man if only there was a spec for that
https://github.com/microsoft/terminal/pull/2193/files#diff-babe43bd5e22464667ee68636e003085R333-R355
I feel silly for having specced something you did already. I’m sorry about that. I couldn’t in truth remember if we landed on that discussion 😅 |
### Compatibility | ||
|
||
A change in profile name, especially one that happens after the product ships | ||
(due to a changing i18n setting on the system), will impact users accustomed to |
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.
i18n
?
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.
Industry-accepted term for “Internationalization”
I'm confused by something. So I'm just going to run through a scenario and you can correct me where I'm wrong.
Separately, what I'm confused about is, can't we just use your And we wouldn't have to worry about |
I really like that. If we stamp the localized string straight into If we go ahead with having Alternatively we could just leave it as |
I’m more concerned about growing the manually-tended list of profile token replacements, if I’m honest. It’s fine for this one-off, but we do need a solution that’ll work for whatever other user-facing strings go in the settings file eventually. If consensus is that we should just do one more one time replacement, I’m down for that. |
Summary of the Pull Request
Pursuant to #4476, we may need to localize the name of the "cmd" profile.
References
#4476