8000 Use ISOWeek for week numbers in Get-Date by paalbra · Pull Request #11536 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]
Skip to content

Use ISOWeek for week numbers in Get-Date #11536

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jan 14, 2020
Merged

Use ISOWeek for week numbers in Get-Date #11536

merged 3 commits into from
Jan 14, 2020

Conversation

paalbra
Copy link
Contributor
@paalbra paalbra commented Jan 9, 2020 •
edited by iSazonov
Loading

PR Summary

In #6542 we had to use a workaround to get a week of the year because .Net Core did not support the feature.

As result Get-Date -UFormat "%V" returns wrong week numbers around new year in some cases mentioned in issue #11534 .

Now we have ISOWeek API in .Net Core 3.1. The PR removes our workaround and utilizes new ISOWeek.GetWeekOfYear() method.

PR Context

This PR fixes #11534 .

PR Checklist

@ghost ghost assigned anmenaga Jan 9, 2020
@paalbra
Copy link
Contributor Author
paalbra commented Jan 9, 2020

You can produce a test by comparing these two (assuming that python is 100% correct):

$dt = Get-Date "1900-01-01"
for ($i = 0; $i -lt 100000; $i++)
{
    echo ("{0:00} {1}" -f (get-date $dt -UFormat "%V"), $dt.ToString("yyyy-MM-dd"));
    $dt = $dt.AddDays(1);
}
import datetime

dt = datetime.datetime(1900, 1, 1)

for _ in range(100000):
    print(dt.strftime("%V %Y-%m-%d"))
    dt += datetime.timedelta(days=1)

@paalbra
Copy link
Contributor Author
paalbra commented Jan 9, 2020

I'm not really sure what to do with the unchecked boxes. Guidance is wanted.

@paalbra
Copy link
Contributor Author
paalbra commented Jan 9, 2020

If you run the above PowerShell code with the current master and with this PR fix you will get this diff:

117 week numbers are wrong in these ~300 years.

$ sdiff -s outcurrent.txt outfixed.txt -w 29
53 1902-01-03 | 01 1902-01-03
53 1903-01-02 | 01 1903-01-02
53 1903-01-03 | 01 1903-01-03
53 1908-01-03 | 01 1908-01-03
53 1913-01-03 | 01 1913-01-03
53 1914-01-02 | 01 1914-01-02
53 1914-01-03 | 01 1914-01-03
53 1919-01-03 | 01 1919-01-03
53 1920-01-02 | 01 1920-01-02
53 1920-01-03 | 01 1920-01-03
53 1925-01-02 | 01 1925-01-02
53 1925-01-03 | 01 1925-01-03
53 1930-01-03 | 01 1930-01-03
53 1931-01-02 | 01 1931-01-02
53 1931-01-03 | 01 1931-01-03
53 1936-01-03 | 01 1936-01-03
53 1941-01-03 | 01 1941-01-03
53 1942-01-02 | 01 1942-01-02
53 1942-01-03 | 01 1942-01-03
53 1947-01-03 | 01 1947-01-03
53 1948-01-02 | 01 1948-01-02
53 1948-01-03 | 01 1948-01-03
53 1953-01-02 | 01 1953-01-02
53 1953-01-03 | 01 1953-01-03
53 1958-01-03 | 01 1958-01-03
53 1959-01-02 | 01 1959-01-02
53 1959-01-03 | 01 1959-01-03
53 1964-01-03 | 01 1964-01-03
53 1969-01-03 | 01 1969-01-03
53 1970-01-02 | 01 1970-01-02
53 1970-01-03 | 01 1970-01-03
53 1975-01-03 | 01 1975-01-03
53 1976-01-02 | 01 1976-01-02
53 1976-01-03 | 01 1976-01-03
53 1981-01-02 | 01 1981-01-02
53 1981-01-03 | 01 1981-01-03
53 1986-01-03 | 01 1986-01-03
53 1987-01-02 | 01 1987-01-02
53 1987-01-03 | 01 1987-01-03
53 1992-01-03 | 01 1992-01-03
53 1997-01-03 | 01 1997-01-03
53 1998-01-02 | 01 1998-01-02
53 1998-01-03 | 01 1998-01-03
53 2003-01-03 | 01 2003-01-03
53 2004-01-02 | 01 2004-01-02
53 2004-01-03 | 01 2004-01-03
53 2009-01-02 | 01 2009-01-02
53 2009-01-03 | 01 2009-01-03
53 2014-01-03 | 01 2014-01-03
53 2015-01-02 | 01 2015-01-02
53 2015-01-03 | 01 2015-01-03
53 2020-01-03 | 01 2020-01-03
53 2025-01-03 | 01 2025-01-03
53 2026-01-02 | 01 2026-01-02
53 2026-01-03 | 01 2026-01-03
53 2031-01-03 | 01 2031-01-03
53 2032-01-02 | 01 2032-01-02
53 2032-01-03 | 01 2032-01-03
53 2037-01-02 | 01 2037-01-02
53 2037-01-03 | 01 2037-01-03
53 2042-01-03 | 01 2042-01-03
53 2043-01-02 | 01 2043-01-02
53 2043-01-03 | 01 2043-01-03
53 2048-01-03 | 01 2048-01-03
53 2053-01-03 | 01 2053-01-03
53 2054-01-02 | 01 2054-01-02
53 2054-01-03 | 01 2054-01-03
53 2059-01-03 | 01 2059-01-03
53 2060-01-02 | 01 2060-01-02
53 2060-01-03 | 01 2060-01-03
53 2065-01-02 | 01 2065-01-02
53 2065-01-03 | 01 2065-01-03
53 2070-01-03 | 01 2070-01-03
53 2071-01-02 | 01 2071-01-02
53 2071-01-03 | 01 2071-01-03
53 2076-01-03 | 01 2076-01-03
53 2081-01-03 | 01 2081-01-03
53 2082-01-02 | 01 2082-01-02
53 2082-01-03 | 01 2082-01-03
53 2087-01-03 | 01 2087-01-03
53 2088-01-02 | 01 2088-01-02
53 
8000
2088-01-03 | 01 2088-01-03
53 2093-01-02 | 01 2093-01-02
53 2093-01-03 | 01 2093-01-03
53 2098-01-03 | 01 2098-01-03
53 2099-01-02 | 01 2099-01-02
53 2099-01-03 | 01 2099-01-03
53 2105-01-02 | 01 2105-01-02
53 2105-01-03 | 01 2105-01-03
53 2110-01-03 | 01 2110-01-03
53 2111-01-02 | 01 2111-01-02
53 2111-01-03 | 01 2111-01-03
53 2116-01-03 | 01 2116-01-03
53 2121-01-03 | 01 2121-01-03
53 2122-01-02 | 01 2122-01-02
53 2122-01-03 | 01 2122-01-03
53 2127-01-03 | 01 2127-01-03
53 2128-01-02 | 01 2128-01-02
53 2128-01-03 | 01 2128-01-03
53 2133-01-02 | 01 2133-01-02
53 2133-01-03 | 01 2133-01-03
53 2138-01-03 | 01 2138-01-03
53 2139-01-02 | 01 2139-01-02
53 2139-01-03 | 01 2139-01-03
53 2144-01-03 | 01 2144-01-03
53 2149-01-03 | 01 2149-01-03
53 2150-01-02 | 01 2150-01-02
53 2150-01-03 | 01 2150-01-03
53 2155-01-03 | 01 2155-01-03
53 2156-01-02 | 01 2156-01-02
53 2156-01-03 | 01 2156-01-03
53 2161-01-02 | 01 2161-01-02
53 2161-01-03 | 01 2161-01-03
53 2166-01-03 | 01 2166-01-03
53 2167-01-02 | 01 2167-01-02
53 2167-01-03 | 01 2167-01-03
53 2172-01-03 | 01 2172-01-03

@vexx32
Copy link
Collaborator
vexx32 commented Jan 9, 2020

@paalbra it may be worth filing a documentation issue so that we can make a note in the docs and users are aware that this functionality was broken and in which versions. Is it broken in Windows PowerShell as well, or was it broken sometime during 6.x?

I would definitely recommend adding some tests as well -- perhaps test a handful of dates that are known to be affected, as well as testing perhaps at least a few weeks' worth of dates from a few different years so we have a decent test surface for the feature and can catch regressions if they occur.

If you would like to test dates from a whole century, be my guess -- just be mindful of the runtime of those tests. If it's not significant, it may be worth doing, if a little bit excessive. At the least, we should definitely test the cases we know were previously broken in the last century or so and a handful of "control" cases just to be sure.

Let us know if you have any questions / need some pointers. 🙂

@paalbra
Copy link
Contributor Author
paalbra commented Jan 9, 2020

@vexx32 Thanks for the reply. Adding some more docs is a good idea (dates can be pretty confusing if you're not really precise). I don't think %V ever has worked properly/as in ISO8601.

I do believe that Windows Powershell (my Windows 10 has 5.1.18362.145) and PowerShell 6.0.0 does a simple (dateTime.DayOfYear / 7) + 1 which is kind of bad (and it doesn't zero prefix like the docs says it should). I've tested with the following and it produces no output in these two versions:

$date = Get-Date "1900-01-01"
$count = 0
while ($count -lt 100000){
    $formatdate = Get-Date $date -UFormat '%Y-%m-%d : %V'
    $week1 = Get-Date $date -UFormat '%V'
    $week2 = [string] ([math]::Floor(($date.DayOfYear / 7) + 1))
    if ($week1 -ne $week2) {
        Write-Output "$formatdate : ($week1 != $week2)"
    }
    $date = $date.AddDays(1)
    $count += 1
}

The current behavior of master branch is better, but has the bug mentioned. Looks like this behavior was added in #6542, but I'm not sure which PowerShell 6.x this got released in? How can I find out?

More tests is also a good idea. I'll look into that. I think adding at least some of the dates in the sdiff output above might be enough.

@paalbra
Copy link
Contributor Author
paalbra commented Jan 9, 2020 •
edited
Loading

I've added tests for some dates that triggers this issue.

Also: @KalleOlaviNiemitalo has mentioned (in the issue) that System.Globalization.ISOWeek (with GetWeekOfYear(DateTime)) exist in .NET Core 3.1. This is of course a much better solution than the current custom calculation. I've added a commit using this method.

@paalbra
Copy link
Contributor Author
paalbra commented Jan 10, 2020

Regarding the change in 6bd70bc .

I'm not really sure how to find out when this was merged in a release, but I believe that this command might be correct?

$ git describe --contains 6bd70bc2d065918c03228f54befc3ed8cf1bfc59
v6.1.0-preview.2~36

I.e. it was released in 6.1.0

@paalbra paalbra changed the title Fixes calculation of week numbers in Get-Date Use ISOWeek for week numbers in Get-Date Jan 10, 2020
@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Jan 10, 2020
@iSazonov iSazonov added this to the GA-consider milestone Jan 10, 2020
@iSazonov iSazonov added the CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log label Jan 10, 2020
@iSazonov
Copy link
Collaborator

@paalbra Thanks for your contribution!

@SteveL-MSFT Formally it is a breaking change but I think it is right thing to get in 7.0 GA - we follow .Net Core behavior and ISO standard.

@anmenaga anmenaga merged commit aada0a8 into PowerShell:master Jan 14, 2020
daxian-dbw pushed a commit that referenced this pull request Jan 14, 2020
@paalbra @daxian-dbw
Use ISOWeek for week numbers in Get-Date (#11536)
7f31fd5
@daxian-dbw daxian-dbw modified the milestones: GA-approved, 7.0.0-rc.2 Jan 14, 2020
@ghost
Copy link
ghost commented Jan 16, 2020

🎉v7.0.0-rc.2 has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request Jan 16, 2020
Closed
@TravisEz13 TravisEz13 mentioned this pull request Oct 12, 2022
Merged
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SteveL-MSFT SteveL-MSFT SteveL-MSFT approved these changes

@iSazonov iSazonov iSazonov approved these changes

Assignees

@anmenaga anmenaga

Labels
CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log
Projects
None yet
Milestone
7.0.0-rc.2
Development

Successfully merging this pull request may close these issues.

Get-Date -UFormat "%V" returns wrong weeknumber
7 participants
@paalbra @vexx32 @iSazonov @SteveL-MSFT @daxian-dbw @anmenaga @adityapatwardhan
0