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.

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

Update to allow more timezone patterns #653

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Update to allow more timezone patterns #653

wants to merge 5 commits into from

Conversation

dalrrard
Copy link

@dalrrard dalrrard commented Apr 22, 2022 •
edited by Taarini
Loading

Modify regex to allow timezones that use UTC offset.

Description

Motivation and Context

Impact (If any)

Screenshots:

Checklist:

  • I have updated the changelog.
  • I have updated the documentation (If applicable).
  • I have added tests to cover my changes (If applicable).
  • All new and existing tests passed.
  • All new code passed compilation.

@dalrrard
Update to allow more timezone patterns
1c0465c 
Modify regex to allow timezones that use UTC offset.
@dalrrard dalrrard requested a review from a team as a code owner April 22, 2022 13:16
danielgraziano
danielgraziano suggested changes Apr 25, 2022
View reviewed changes
Copy link
Contributor

@danielgraziano danielgraziano left a comment

Choose a reason for hiding this comment

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

Please add a changelog and unittests

@sjpatel21 sjpatel21 requested a review from Taarini May 27, 2022 20:45
Taarini
Taarini requested changes May 30, 2022
View reviewed changes
Copy link
Contributor

@Taarini Taarini left a comment

Choose a reason for hiding this comment

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

Can you please add change log

lsheikal
lsheikal requested changes Jun 13, 2022
View reviewed changes
Copy link
Contributor

@lsheikal lsheikal left a comment

Choose a reason for hiding this comment

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

@dalrrard please address comments, if no response will have to close this PR.

@dalrrard
Copy link
Author

@Taarini I've added the requested changelog

@dalrrard dalrrard requested a review from Taarini June 14, 2022 16:56
Taarini
Taarini approved these changes Jun 14, 2022
View reviewed changes
Copy link
Contributor

@Taarini Taarini left a comment

Choose a reason for hiding this comment

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

Please add unit test if possible

Taarini
Taarini requested changes Jun 14, 2022
View reviewed changes
Copy link
Contributor

@Taarini Taarini left a comment

Choose a reason for hiding this comment

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

Please resolve the conflicts

@dalrrard
Copy link
Author

I don't have the time to make unit tests for this. I can close this PR and open an issue instead if you'd like.

@dalrrard
Copy link
Author

Conflicts have been resolved.

Taarini
Taarini approved these changes Jul 15, 2022
View reviewed changes
Copy link
Contributor

@Taarini Taarini left a comment

Choose a reason for hiding this comment

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

If there was a unit test for show clock, it would have been better. Please try to add since it won't take much of time

@GerriorL GerriorL requested a review from lsheikal August 11, 2022 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielgraziano danielgraziano danielgraziano requested changes

@Taarini Taarini Taarini approved these changes

@lsheikal lsheikal lsheikal approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dalrrard @danielgraziano @Taarini @lsheikal @GerriorL