-
Notifications
You must be signed in to change notification settings - Fork 9
Add named params and yard docs to create_milestone
method at GithubHelper
#426
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
Merged
AliSoftware
merged 11 commits into
wordpress-mobile:trunk
from
raafaelima:add/named-params-create-milestone
Nov 17, 2022
Merged
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
66ccfcf
GithubHelperSpec: Add Yard Docs and Named Parameters to create_milest…
raafaelima b658f87
CreateNewMilestoneAction: Adds named parameters to create_milestone G…
raafaelima 10528de
GithubHelper#create_milestone: Rephrase yard docs
raafaelima 255d108
GithubHelper#create_milestone: Rename parameters to be more descriptive
raafaelima 99a95ac
CreateNewMilestoneAction: Rephrase comment about release times to be …
raafaelima 39b23c8
GithubHelper#create_milestone: Add unit tests and validation to date …
raafaelima 0750081
Merge branch 'trunk' into add/named-params-create-milestone
raafaelima b218ccc
GithubHelper: Fix Timezone changes unit tests
raafaelima de90d4d
GithubHelper: Adds more unit tests to validate the Timezone changes
raafaelima d87c03c
GithubHelper: Fix Rubocop Issues
raafaelima 7f99d51
GithubHelperSpec: Adds comments to DST cases to be more clear and new…
raafaelima File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice edge case of having
2022-03-27 23:00:00Z
being exactly2022-03-28 00:00:00 +0100
because that day is the exact day of the DST change, when London has switched to+0100
… while for your next test case on line 259, the2022-03-26 23:00:00Z
date still corresponds to2022-03-26 23:00:00 +0000
because this is just before the DST change when London were still on+0000
…Took me a while to figure that out though and to triple-check that this difference between the
due_date
(2022-03-27
) and thedue_on
(2022-03-28
) was the correct and expected behavior here, and not an incorrect value due to our date math being buggy or whatnot. Phew!Could maybe benefit from some small code comment (here and on line 259) to give more context about those tricky details?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For sure! 😄
Besides the comments, I also added another test to cover the scenario of the
due_date
is not close enough to the end of the day to exist a date change (Line 259) and also add a new case, to test what happens if you have a date close to the day change, but not in DST (Line 278).