Skip to content

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 force-pushed the bugifx/issue#1167 branch from 24355ca to 595beb0 Compare August 29, 2021 11:37
@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