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

[Fix #336] coercion from interval to period #347

Closed
wants to merge 2 commits into from
Closed

[Fix #336] coercion from interval to period #347

wants to merge 2 commits into from

Conversation

AlunHewinson
Copy link

If the sign of the @month and @day slots of the new period are opposite to each other, set @day to zero.
#336

It is my understanding that negative periods are legal but a period created from an interval should not have a negative month slot and positive days or vice versa.

First attempted in pull request #346 but there was an error in the test and it didn't handle NAs. Sorry if it fails the testthat tests; I need to learn how to understand and run those myself locally. Hope you take this attempted bugfix in good faith if it doesn't work; I won't try again if it fails.

If the sign of the month and day of the new period are opposite to each other, set @day to zero.

#336
@vspinu
Copy link
Member

vspinu commented Sep 29, 2015

I don't understand why you are setting day to 0. If the day componnet is negative, it's negative for a reason or not? Something is going wrong with the logic in that function. You cannot just overwrite it with 0 and label as fixed.

I will try to have a look at it today.

(You don't need close an issue and open a new one each time you change a thing. Simply git push --force to the same branch.)

@vspinu
Copy link
Member

vspinu commented Sep 30, 2015

Fixed in 0bbe8c1

@vspinu vspinu closed this Sep 30, 2015
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.

2 participants