Skip to content
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

accept timezone for timestamps #1432

Merged
merged 1 commit into from Jul 10, 2022
Merged

Conversation

julian7
Copy link
Contributor

@julian7 julian7 commented Jul 9, 2022

What type of PR is this?

  • feature

What this PR does / why we need it:

When TimestampFlag is used with implicit timezone, default time.Parse behavior is assumed, which is parsing time in some timezone, and provide a UTC time. I haven't tested it myself, but I've read somewhere this works just with local timezone on windows, but it does this in UTC on linux. Either way, time.Parse assumes UTC times on linux and mac.

The suggested solution is to use time.ParseInLocation instead, and provide a *time.Location value for the default timezone.

This PR adds a Timezone member into TimestampFlag struct. Input parsing runs time.ParseInLocation when location is present, but keeps old behavior if Timezone is nil, to guarantee backwards compatibility.

Which issue(s) this PR fixes:

I haven't opened a separate issue for this PR.

Special notes for your reviewer:

I haven't included v2approve output.

Testing

This PR adds a new test, TestTimestampFlagApply_Timezoned (copied from TestTimestampFlagApply), which constructs a static timezone called PDT (UTC-7h), and expects the example UTC time provided in PDT timezone.

Release Notes

Add optional timezone (*time.Location) to TimezoneFlag to assume other timezones than UTC

@julian7 julian7 requested a review from a team as a code owner July 9, 2022 19:27
Copy link
Member

@meatballhat meatballhat left a comment

Choose a reason for hiding this comment

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

Lovely! Thank you 🎉

@meatballhat meatballhat merged commit 2e71cb8 into urfave:main Jul 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants