-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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
Allow partial culture matching in Update-Help #18037
Allow partial culture matching in Update-Help #18037
Conversation
There is a single test failing, looks like some empty case for |
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
Looks like the failing test case was because of a typo in |
Unrelated and style changes should be moved to separate PRs. |
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
@adityapatwardhan Can I please get this one reviewed? I previously got approve from you on #17780 , this one is related. Getting a bit dusty 😅 |
@adityapatwardhan Can you please review this PR? |
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
I will bring this up in the next Cmdlet WG meeting this week. No promises, but I'll try! |
The Cmdlet working group has reviewed this PR and agrees with the proposed behaviour. There is possibly a wider discussion to be had over help fallback culture in general. |
/// zh-Hans-CN => { zh-Hans-CN, zh-Hans, zh }. | ||
/// </example> | ||
/// <returns>An enumerable list of culture names.</returns> | ||
internal static IEnumerable<string> GetCultureFallbackChain(CultureInfo culture) |
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.
One of the issues is that on Ubuntu 20+, the default culture is set as C.UTF-8.
It doesn't have 'en' in its parent chain. Can we accommodate for cases like that? If a matching culture is not found fallback to en-US?
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.
@adityapatwardhan Interesting case, but out of scope. From my skimming of the POSIX standard, C.UTF-8
is more of a default null culture and in no way equivalent to en-US
. In fact, presuming everyone uses en-US
is the very source of the issues I am attempting to fix.
Source issue shows the error that will be shown, I think it is well good enough:
Update-Help: Failed to update Help for the module(s) 'Microsoft.PowerShell.Core' with UI culture(s) {C}:
Unable to retrieve the HelpInfo XML file for UI culture C.
Make sure the HelpInfoUri property in the module manifest is valid or check your network connection and then try the command again..
English-US help content is available and can be installed using: Update-Help -UICulture en-US.
I could specialise the error message to also show something like "Your culture is C.UTF-8, which is a default not associated with any language, consider changing your system culture", but please create another issue for it so that it can be discussed.
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.
I think the updated error message is much better than the current one. It would be great if you could do that. I will open a issue for that.
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.
Here: #19633
@dkaszews Thank you for your contribution! |
@dkaszews @adityapatwardhan Can one of you file a docs issue for this change? |
🎉 Handy links: |
PR Summary
Allows the help locale to match, if it is in the list of parents (fallback chain) of the provided (or implicit) one.
Fixes #18021
PR Context
Currently, if the help has locale
'en-US'
as most do, only the following will work and everything else will throw an error:The PR changes it so that the following will also work:
Explicit
Update-Help -UICulture 'en-GB'
will still not work, because explicit culture does not use fallback chain.This solves the bug of fallback chain not actually working, as fallback culture
'en'
did not match anything. It was supposed to work by use of globbing, but lack of+ "*"
meant that the glob still used exact matching. It also allows the user to use explicit-UICulture 'en'
to mean "any English will do, regardless of region".PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.(which runs in a different PS Host).