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

feat(civil): before and after methods for Time #5703

Merged
merged 5 commits into from May 24, 2022
Merged

feat(civil): before and after methods for Time #5703

merged 5 commits into from May 24, 2022

Conversation

mohamadHarith
Copy link
Contributor

closes #5702

@mohamadHarith mohamadHarith requested a review from a team as a code owner February 27, 2022 06:22
@google-cla
Copy link

google-cla bot commented Feb 27, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

@product-auto-label product-auto-label bot added the size: s Pull request size is small. label Feb 27, 2022
@product-auto-label product-auto-label bot added the stale: old Pull request is old and needs attention. label Mar 29, 2022
@codyoss codyoss requested a review from quartzmo April 20, 2022 19:38
@product-auto-label product-auto-label bot added stale: extraold Pull request is critically old and needs prioritization. and removed stale: old Pull request is old and needs attention. labels Apr 28, 2022
@mohamadHarith
Copy link
Contributor Author

Hi @codyoss @quartzmo , would highly appreciate if you can review and accept my PR. My PR is useful for comparing the Time in its chronological order.

quartzmo
quartzmo requested changes May 5, 2022
View reviewed changes
Copy link
Member

@quartzmo quartzmo left a comment

Choose a reason for hiding this comment

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

@mohamadHarith Sorry about the delay, and thank you for this PR! I have made a few small change requests. Also, I am wondering if your new methods should be included in DateTime.Before and DateTime.After? If you modified these methods to compare times in addition to dates, would that be a breaking change? Or could it be seen as a fix (patch)? (The same dates with different times would then return true for one method or the other; I believe they currently return false for both.)

civil/civil.go Show resolved Hide resolved
civil/civil.go Show resolved Hide resolved
civil/civil_test.go
{Time{12, 0, 0, 0}, Time{14, 0, 0, 0}, true},
{Time{12, 20, 0, 0}, Time{12, 30, 0, 0}, true},
{Time{12, 20, 10, 0}, Time{12, 20, 20, 0}, true},
{Time{12, 20, 10, 5}, Time{12, 20, 10, 10}, true},
Copy link
Member

Choose a reason for hiding this comment

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

Can you please add a test case for equal times: {Time{12, 20, 10, 5}, Time{12, 20, 10, 5}, false},

civil/civil_test.go
{Time{12, 0, 0, 0}, Time{14, 0, 0, 0}, false},
{Time{12, 20, 0, 0}, Time{12, 30, 0, 0}, false},
{Time{12, 20, 10, 0}, Time{12, 20, 20, 0}, false},
{Time{12, 20, 10, 5}, Time{12, 20, 10, 10}, false},
Copy link
Member

Choose a reason for hiding this comment

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

Can you please add a test case for equal times: {Time{12, 20, 10, 5}, Time{12, 20, 10, 5}, false},

civil/civil_test.go
{Time{12, 20, 10, 5}, Time{12, 20, 10, 10}, false},
} {
if got := test.t1.After(test.t2); got != test.want {
t.Errorf("%v.Before(%v): got %t, want %t", test.t1, test.t2, got, test.want)
Copy link
Member

Choose a reason for hiding this comment

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

Before should be After in this message.

@quartzmo
Copy link
Member

quartzmo commented May 5, 2022

I am wondering if your new methods should be included in DateTime.Before and DateTime.After? If you modified these methods to compare times in addition to dates, would that be a breaking change? Or could it be seen as a fix (patch)?

@codyoss and I discussed this and we agreed that DateTime.Before and DateTime.After should have always included Time in their comparisons and that updating them to depend on your new methods can be considered a fix.

However, you do not need to update DateTime.Before and DateTime.After in this PR, which should remain focused on Time. We will fix DateTime.Before and DateTime.After in a later PR. Thank you!

@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels May 23, 2022
@quartzmo quartzmo added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 23, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 23, 2022
@quartzmo quartzmo added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 24, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 24, 2022
@quartzmo quartzmo added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 24, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 24, 2022
@quartzmo quartzmo merged commit 7acaaaf into googleapis:main May 24, 2022
@release-please release-please bot mentioned this pull request May 24, 2022
Merged
@quartzmo
Copy link
Member

@mohamadHarith Thank you for this contribution! ❤️

@quartzmo
Copy link
Member

DateTime.Before and DateTime.After should have always included Time in their comparisons

I was mistaken, DateTime.Before uses DateTime.In to convert DateTime to time.Date (including time fields) for comparison that includes the Time component.

quartzmo added a commit to quartzmo/google-cloud-go that referenced this pull request May 24, 2022
@quartzmo
chore(civil): refactor methods to use consistent receiver names
3913eec 
refs: googleapis#5703
@quartzmo quartzmo mentioned this pull request May 24, 2022
Merged
quartzmo added a commit that referenced this pull request May 24, 2022
@quartzmo
chore(civil): refactor methods to use consistent receiver names (#6089)
b01df2e 
refs: #5703
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@quartzmo quartzmo quartzmo approved these changes

Assignees
No one assigned
Labels
size: m Pull request size is medium. stale: extraold Pull request is critically old and needs prioritization.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

civil: Before and After methods for Time
3 participants
@mohamadHarith @quartzmo @kokoro-team