-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Dynamically evaluate width of LastWriteTime for formatting output on Unix #24624
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
Dynamically evaluate width of LastWriteTime for formatting output on Unix #24624
Conversation
| // Widest locales for date serialization are hr-HR, hu-HU, bg-BG (13 & 14). | ||
| // width as a constant should not be less than 20 | ||
| // https://github.com/PowerShell/PowerShell/issues/19723 | ||
| .AddHeader(Alignment.Right, label: "LastWriteTime", width: 20) |
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 fix 23 languages and breaks (add extra spaces) over 750 languages. I think this is unacceptable.
Perhaps we could take the format of the current culture and calculate its size dynamically.
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.
Is this more along the lines of what you were thinking?
1319527 to
c42bd90
Compare
| .AddHeader(Alignment.Right, label: "LastWriteTime", width: 16) | ||
| // Until LastWriteTime prints as '{0:d} {0:HH}:{0:mm}', it will always be local date + 6 wide. | ||
| // https://github.com/PowerShell/PowerShell/issues/19723 | ||
| .AddHeader(Alignment.Right, label: "LastWriteTime", width: CultureInfo.CurrentCulture.DateTimeFormat.ShortDatePattern.Length + 6) |
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.
Is this works for all cultures?
Full list unique formats you can get with:
(Get-Culture -ListAvailable).DateTimeFormat.ShortDatePattern | Sort -UniqueThere 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'm having trouble building the project as it is. I opened it in the provided container, installed the suggested extensions, rebased my work on latest master, set PS as the default project, run the build task, and it fails to compile.
VERBOSE: Using default feeds which are Microsoft, use -UseNuGetOrg to switch to Public feeds
VERBOSE: In Find-DotNet
VERBOSE: Executing: dotnet --list-sdks
VERBOSE: Find-DotNet: dotnetCLIInstalledVersion = 10.0.100-rc.1.25451.107; chosenDotNetVersion = 10.0.100-rc.1.25451.107
VERBOSE: Using configuration 'Debug'
VERBOSE: Using framework 'net10.0'
VERBOSE: Using runtime 'linux-x64'
VERBOSE: Top project directory is /PowerShell/src/powershell-unix
VERBOSE: Building without shim
Run dotnet publish /property:GenerateFullPaths=true /property:ErrorOnDuplicatePublishOutputFiles=false --output /PowerShell/debug --self-contained /property:IsWindows=false /property:AppDeployment=SelfContained --configuration Debug --framework net10.0 --runtime linux-x64 /property:SDKToUse=Microsoft.NET.Sdk from /PowerShell/src/powershell-unix
PSVersionInfoGenerator succeeded (1.4s) → /PowerShell/src/System.Management.Automation/SourceGenerators/PSVersionInfoGenerator/bin/Debug/netstandard2.0/SMA.Generator.dll
System.Management.Automation failed with 57 error(s) (17.3s)
/PowerShell/src/System.Management.Automation/engine/hostifaces/HostUtilities.cs(100,70): error CS0117: 'SuggestionStrings' does not contain a definition for 'Suggestion_CommandExistsInCurrentDirectory_Legacy'
/PowerShell/src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs(6241,47): error CS0117: 'TabCompletionStrings' does not contain a definition for 'RequiresModulesParameterDescription'
/PowerShell/src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs(6242,49): error CS0117: 'TabCompletionStrings' does not contain a definition for 'RequiresPSEditionParameterDescription'
/PowerShell/src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs(6243,58): error CS0117: 'TabCompletionStrings' does not contain a definition for 'RequiresRunAsAdministratorParameterDescription'
I don't really know how to fix this.
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.
Start-PSBuild -ResGen
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 have done extensive testing for all locales, not just those with the latin alphabet. I have managed to all non-Arabic locales, which is an improvement, albeit not a complete solution. Arabic locales are currently broken on Unix even without this change, so if nothing else, it doesn't make things worse.
The current strategy of trying to devise the width of the table statically is kind-of backwards. Trying to relate the ShortDatePattern (The {0:d} part of the format string) to the resulting string seems kind-of underwhelming too, because a lot of locales have singular day and month designators/specifiers, which results in shorter anticipated date lengths than the actual ones, even if a single entry in a folder was last written on a day or month with double digits. Some locales even incorporate the era name in their ShortDatePattern which make things even more complicated.
I tested using this PS script:
(Get-Culture -ListAvailable) | % { if ([String]::Format( $_, '{0:d} {0:HH}:{0:mm}', (Get-Date)).Length -gt ($_.DateTimeFormat.ShortDatePattern.Length + (($_.DateTimeFormat.ShortDatePattern.ToCharArray() | where {$_ -eq 'd'}).count -eq 1 ? 1 : 0 ) + (($_.DateTimeFormat.ShortDatePattern.ToCharArray() | where {$_ -eq 'M'}).count -eq 1 ? 1 : 0 ) + (($_.DateTimeFormat.ShortDatePattern.ToCharArray() | where {$_ -eq 'g'}).count -eq 1 ? ($_.DateTimeFormat.GetEraName("1").Length + 1) : 0) + 6)) { Write-Host $_.Name 'has pattern:' $_.DateTimeFormat.ShortDatePattern } }which is the PS port of the C# logic found in the PR. Only Arabic locales fall short in terms of the calculated column widths. The remaining issue must lie with right justification/alignment, but I can't figure out why the output is garbled the way it is.
c42bd90 to
b91574c
Compare
| (CultureInfo.CurrentCulture.DateTimeFormat.ShortDatePattern.AsSpan().Count('d') == 1 ? 1 : 0) + | ||
| (CultureInfo.CurrentCulture.DateTimeFormat.ShortDatePattern.AsSpan().Count('M') == 1 ? 1 : 0) + | ||
| (CultureInfo.CurrentCulture.DateTimeFormat.ShortDatePattern.AsSpan().Count('g') == 1 ? 1 + CultureInfo.CurrentCulture.DateTimeFormat.GetEraName(1).Length : 0) + | ||
| 6) |
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.
Hmm, why not test real value? I mean evaluate something like Datetime.Utc.ToString().Length.
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.
@iSazonov Indeed, that gave a good idea. Utc has seconds at the end, it's length is not fixed... BUT, all I really needed was the longest possible time string formatted as {0:d} {0:HH}:{0:mm} in the current locale. Enter Calendar.MaxSupportedDateTime 🎆. It is guaranteed to have as many year, month, day, hour and minutes digits as possible.
|
@MathiasMagnus LGTM. Please add screenshots in the PR description for hu_HU.UTF-8 (and maybe some other cultures) before and after the PR. |
|
/azp run PowerShell-Windows-Packaging-CI, PowerShell-CI-linux-packaging |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
|
📣 Hey @@MathiasMagnus, how did we do? We would love to hear your feedback with the link below! 🗣️ 🔗 https://aka.ms/PSRepoFeedback |
|
@MathiasMagnus Thank for your contribution and patience! |
PR Summary
Fixes #19723
As per @iSazonov 's request, here is a screenshot of some output freom before and after the PR in en-US and a few trickier locales, including the motivating hu-HU locale. The table width visibly adapts to the time format length.
PR Context
The issue which was unresolved has since been aggravated by another PR #18183. This PR adds the required characters to the LastWriteTime column to make it render correctly on all locales.
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.- [ ] Issue filed:
(which runs in a different PS Host).