Skip to content

WIP: Fix Get-Date -UFormat %V week number output #4508

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
Closed
timparkinson wants to merge 8 commits into from
Closed

WIP: Fix Get-Date -UFormat %V week number output #4508

timparkinson wants to merge 8 commits into from

Conversation

@timparkinson
Copy link

@timparkinson timparkinson commented Aug 5, 2017 •
edited by TravisEz13
Loading

PR Summary

Fix #4264 makes week number output compliant with ISO-8601.

PR Checklist

iSazonov
iSazonov reviewed Aug 5, 2017
View reviewed changes
Copy link
Collaborator

@iSazonov iSazonov left a comment

Choose a reason for hiding this comment

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

@timparkinson Thanks for your contribution!

Could you please review all uformat patterns - it seems we should fix all.

src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetDateCommand.cs Outdated
Copy link
Collaborator

@iSazonov iSazonov Aug 5, 2017

Choose a reason for hiding this comment

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

I believe we shouldn't create new object.
System.Threading.Thread.CurrentThread.CurrentCulture.Calendar

Copy link
Author

@timparkinson timparkinson Aug 7, 2017

Choose a reason for hiding this comment

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

I don't believe using the CurrentCulture is appropriate in this case, because ISO 8601 is Gregorian calendar based and CurrentCulture could be set to a different calendar.

In any case further tests based on the table at https://en.wikipedia.org/wiki/ISO_week_date#Relation_with_the_Gregorian_calendar shows that my initial stab at this was incorrect, because the GetWeekOfYear does not produce the ISO 8601 output for all cases. I'll take a bit of a harder stare at this.

Copy link
Collaborator

@iSazonov iSazonov Aug 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I believe we should follow CoreFX and should be CurrentCulture based not ISO based for all culture.
/cc @SteveL-MSFT

Copy link
Member

@SteveL-MSFT SteveL-MSFT Aug 14, 2017

Choose a reason for hiding this comment

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

Agree with @iSazonov that in general, we should be using CurrentCulture. Not an expert by any means on this topic, but looking at the Wikipedia page, it seems we should implement the formula they have there?

Copy link
Collaborator

@iSazonov iSazonov Aug 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

CoreCLR already implemented this in GregorianCalendar(...)
We can use the implementation.

test/powershell/Modules/Microsoft.PowerShell.Utility/Get-Date.Tests.ps1 Outdated
Copy link
Collaborator

@iSazonov iSazonov Aug 5, 2017

Choose a reason for hiding this comment

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

The tests may fail if current locale is not English. So we shouldn't use explicit constants - please evaluate them.

$a=Get-Culture
$a.Calendar
...

@iSazonov
Copy link
Collaborator

iSazonov commented Aug 18, 2017

@timparkinson Could you please continue?

@timparkinson
Copy link
Author

timparkinson commented Aug 18, 2017

@iSazonov yes, I'm happy to continue looking at this, but I haven't had time over the last week. I may be able to spend a bit of time this weekend.

I did start looking at the other UFormats and there are definitely others that don't match the output from the Linux date command.

Would you prefer me to address them as one PR or create separate issues for each?

@iSazonov
Copy link
Collaborator

iSazonov commented Aug 21, 2017

If you are limited in time you can split the work on some PRs - describe all points in one Issue and fix step by step.

@timparkinson
Copy link
Author

timparkinson commented Aug 22, 2017

@iSazonov OK, I'll raise another issue detailing all the deviations from Linux behaviour in UFormat and then I'll look to fix step by step as you suggest.

I have a mini vacation after today, and will be without computer/internet - I'll hopefully be able to get to it next week.

@iSazonov
Copy link
Collaborator

iSazonov commented Aug 22, 2017

/cc @SteveL-MSFT Changing UFormat looks like a breaking change - should we have the PowerShell committee's approvement?

@SteveL-MSFT SteveL-MSFT added Review - Committee The PR/Issue needs a review from the PowerShell Committee Breaking-Change breaking change that may affect users labels Aug 25, 2017
@joeyaiello
Copy link
Contributor

joeyaiello commented Aug 25, 2017

Backchanneled with @SteveL-MSFT, I'm good with these changes.

@SteveL-MSFT SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Aug 25, 2017
@SteveL-MSFT
Copy link
Member

SteveL-MSFT commented Aug 25, 2017

@PowerShell/powershell-committee discussed this briefly and agree the change makes sense

@iSazonov
Copy link
Collaborator

iSazonov commented Sep 4, 2017

@timparkinson Please rebase.

@timparkinson
Copy link
Author

timparkinson commented Sep 4, 2017

As previously discussed, I intend to raise another issue to document all the UFormat divergences since there are several. Since this PR is incorrect anyway, I guess I should simply close it?

@iSazonov
Copy link
Collaborator

iSazonov commented Sep 4, 2017

It would be right to open the new Issue first and then decide on the fate of that PR. Currently you can rebase:

git chechout <branch>
git pull --rebase PowerShell master
git push -f

@timparkinson timparkinson mentioned this pull request Sep 4, 2017
Closed
14 tasks
Tim Parkinson and others added 8 commits September 4, 2017 12:01
@timparkinson
Fix Get-Date -UFormat '%V' week number handling.
f13a8b5 
Fix PowerShell#4264 makes week number handling compliant with ISO-8601.
@timparkinson
Revert "Fix Get-Date -UFormat '%V' week number handling."
407eb51 
This reverts commit a5886cd5e419e07b884815f740e08b3c69513179.

Solution was incorrect and needs a lot more work.
@timparkinson
Fix Get-Date -UFormat '%V' week number handling.
38eb1f7 
Fix PowerShell#4264 makes week number handling compliant with ISO-8601.
@timparkinson
Revert "Fix Get-Date -UFormat '%V' week number handling."
f952830 
This reverts commit a5886cd5e419e07b884815f740e08b3c69513179.

Solution was incorrect and needs a lot more work.
@timparkinson
Fix Get-Date -UFormat '%V' week number handling.
59af68e 
Fix PowerShell#4264 makes week number handling compliant with ISO-8601.
@timparkinson
Revert "Fix Get-Date -UFormat '%V' week number handling."
7955192 
This reverts commit a5886cd5e419e07b884815f740e08b3c69513179.

Solution was incorrect and needs a lot more work.
@timparkinson
Fix Get-Date -UFormat '%V' week number handling.
b1e4cf9 
Fix PowerShell#4264 makes week number handling compliant with ISO-8601.
@timparkinson
Revert "Fix Get-Date -UFormat '%V' week number handling."
a23b81a 
This reverts commit a5886cd5e419e07b884815f740e08b3c69513179.

Solution was incorrect and needs a lot more work.
@PowerShell PowerShell deleted a comment from msftclas Sep 27, 2017
@lzybkr lzybkr removed their request for review October 10, 2017 02:08
@TravisEz13
Copy link
Member

TravisEz13 commented Mar 14, 2018

This PR appears empty. Can it be closed?

@TravisEz13 TravisEz13 changed the title Fix Get-Date -UFormat '%V' week number output Fix Get-Date -UFormat %V week number output Mar 14, 2018
@TravisEz13 TravisEz13 changed the title Fix Get-Date -UFormat %V week number output WIP: Fix Get-Date -UFormat %V week number output Mar 14, 2018
@timparkinson
Copy link
Author

timparkinson commented Mar 14, 2018

Closing as well out of date.

@iSazonov iSazonov mentioned this pull request Apr 4, 2018
Merged
11 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 left review comments

@iSazonov iSazonov iSazonov left review comments

@adityapatwardhan adityapatwardhan Awaiting requested review from adityapatwardhan

@anmenaga anmenaga Awaiting requested review from anmenaga

@BrucePay BrucePay Awaiting requested review from BrucePay

@charub charub Awaiting requested review from charub

@dantraMSFT dantraMSFT Awaiting requested review from dantraMSFT

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw

@Francisco-Gamino Francisco-Gamino Awaiting requested review from Francisco-Gamino

@JamesWTruher JamesWTruher Awaiting requested review from JamesWTruher

@mirichmo mirichmo Awaiting requested review from mirichmo

@PaulHigin PaulHigin Awaiting requested review from PaulHigin

@TravisEz13 TravisEz13 Awaiting requested review from TravisEz13

@vors vors Awaiting requested review from vors

Assignees

@iSazonov iSazonov

Labels

Breaking-Change breaking change that may affect users Committee-Reviewed PS-Committee has reviewed this and made a decision

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@timparkinson @iSazonov @joeyaiello @SteveL-MSFT @TravisEz13 @msftclas @msftgits