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

Bugfix: Plus/Minus logic failed in the month way when the last day of the month was involved. #1168

Closed
wants to merge 6 commits into from

Conversation

Mifrill
Copy link
Contributor

@Mifrill Mifrill commented Aug 29, 2021

Summary of changes

Fixed issue where compare with relativedelta would erroneously return the wrong last day of the month if the passed argument is the last day of the month.

Closes #1167

Pull Request Checklist

  • Changes have tests
  • Authors have been added to AUTHORS.md
  • News fragment added in changelog.d. See CONTRIBUTING.md for details

@Mifrill Mifrill marked this pull request as ready for review August 29, 2021 11:45
@pganssle
Copy link
Member

I'm kind of surprised that this PR didn't break any existing tests, because the behavior described in #1167 is actually working as intended.

I am not at a computer, so I can try and give a more thorough explanation on that ticket later, but basically the "fall back to last day of month" is only supposed to happen if "same day on a different month" resolves to a date that doesn't exist, so 3/31 + 1 month = 4/30 and 5/31 - 1 month = 4/30, but 4/30 + 1 month = 5/30.

Maybe this PR can be repurposed to add tests enforcing the existing behavior?

@Mifrill
Copy link
Contributor Author

Mifrill commented Aug 29, 2021

Hi, @pganssle

actually working as intended

well, then some of added tests are incorrect, for example:

self.assertEqual(date(2021, 2, 28) + relativedelta(months=1), date(2021, 3, 31))

is it should be?:

self.assertEqual(date(2021, 2, 28) + relativedelta(months=1), date(2021, 3, 28))

but it's really the end of the month, and the end of the next month would be 31, not 28, isn't it?
Same for minus, for example:

self.assertEqual(date(2021, 2, 28) - relativedelta(months=1), date(2021, 1, 31))

is it should be?:

self.assertEqual(date(2021, 2, 28) - relativedelta(months=1), date(2021, 1, 28))

but the last day of Jan is 31 not 28, so it is a bit confusing.

@pganssle
Copy link
Member

Right, this PR and the tests accompanying it are not a desirable change, but the fact that you didn't have to change any existing tests in order to get CI to pass highlights a problem with our existing tests.

We can either close this PR and address that problem at a separate time or if you wanted to, you could change the tests you've added to reflect the actual desired behavior around these things.

The tests themselves will also need to be implemented differently - use pytest style assert statements and parameterize the tests instead of multiple asserts per test.

@Mifrill
Copy link
Contributor Author

Mifrill commented Aug 31, 2021

@pganssle okay, I see the point. I think it's better to close this PR and open a new one with doc type with a pytest style tests suite and some notes for: https://github.com/dateutil/dateutil/blob/master/docs/relativedelta.rst

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.

last day of the month is wrong??
2 participants