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

In months fix #609

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rileyjohngibbs
Copy link

Pull Request Check List

  • Added tests for changed code.
  • Updated documentation for changed code.

This is to address issue #606. The change to the setup fixture in the test_diff module to use UTC instead of Toronto is a workaround for issue #608.

Not sure whether this should be based on develop or master. Some of this is clearly a bug (see February in particular), but some of the side effects are changes in user-facing behavior of what pendulum thinks "1 month" means colloquially, e.g. whether March 31st to April 30th is 1 month (forward), and whether April 30th to March 31st is 1 month (backward).

Rather than try to tweak the logic to satisfy the current notions of what "1 month" means, I let them change and updated the tests to match the new behavior.

@rileyjohngibbs
Copy link
Author

I believe that this change will not count March 31st to April 30th as one month, which is arguable whether it is or not. A check could be added for the following:

Given that the day of the month of the start date exceeds the day of the month of the end date
And the end date is the last day of the month
When Period.in_months is calculated
Then Period.in_months should be calculated as if the end date and start date have the same day of the month.

I don't personally consider this to be any more or less objectively correct than the behavior currently manifested by this PR (because "one month" is not well defined), but it might be more in line with what people "mean" when they say "one month from March 31st."

I can dig back into the C code and make this change if there is a desire for it and it will help this PR along.

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.

1 participant