Skip to content

Conversation

@MathiasMagnus
Copy link
Contributor

@MathiasMagnus MathiasMagnus commented Nov 28, 2024

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.

image

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

@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Nov 28, 2024
@iSazonov iSazonov requested a review from daxian-dbw November 28, 2024 10:21
@microsoft-github-policy-service microsoft-github-policy-service bot added the Review - Needed The PR is being reviewed label Dec 5, 2024
// 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)
Copy link
Collaborator

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.

Copy link
Contributor Author

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?

@MathiasMagnus MathiasMagnus force-pushed the fix-lastwritetime-formatting-on-unix branch from 1319527 to c42bd90 Compare September 17, 2025 07:19
.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)
Copy link
Collaborator

@iSazonov iSazonov Sep 17, 2025

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 -Unique

Copy link
Contributor Author

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Start-PSBuild -ResGen

Copy link
Contributor Author

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.

@MathiasMagnus MathiasMagnus force-pushed the fix-lastwritetime-formatting-on-unix branch from c42bd90 to b91574c Compare October 14, 2025 19:36
(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)
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

@iSazonov
Copy link
Collaborator

@MathiasMagnus LGTM. Please add screenshots in the PR description for hu_HU.UTF-8 (and maybe some other cultures) before and after the PR.

@microsoft-github-policy-service microsoft-github-policy-service bot removed the Review - Needed The PR is being reviewed label Oct 15, 2025
@iSazonov
Copy link
Collaborator

/azp run PowerShell-Windows-Packaging-CI, PowerShell-CI-linux-packaging

@azure-pipelines
Copy link

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@iSazonov iSazonov changed the title Fix LastWriteTime formatting on Unix and locales with wide DateTimeFormat Dynamically evaluate width of LastWriteTime for formatting output on Unix Oct 16, 2025
@iSazonov iSazonov self-assigned this Oct 16, 2025
@iSazonov iSazonov merged commit 9461d96 into PowerShell:master Oct 16, 2025
39 checks passed
@microsoft-github-policy-service
Copy link
Contributor

microsoft-github-policy-service bot commented Oct 16, 2025

📣 Hey @@MathiasMagnus, how did we do? We would love to hear your feedback with the link below! 🗣️

🔗 https://aka.ms/PSRepoFeedback

@iSazonov
Copy link
Collaborator

@MathiasMagnus Thank for your contribution and patience!

SIRMARGIN pushed a commit to SIRMARGIN/PowerShell that referenced this pull request Dec 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

LastWriteTime column width on Linux is set too narrow to support all cultures

2 participants