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

BUG fixes for date_range boundaries #56156

Closed
wants to merge 9 commits into from

Conversation

jrmylow
Copy link
Contributor

@jrmylow jrmylow commented Nov 24, 2023

Fix for two issues with date_range where:

  • A valid timestamp in range would be accidentally dropped when using year end and the starting timestamp was not midnight (56134)
  • A negative year end offset and start increment at a date earlier than year end would include the year end of the starting date (despite being out of bounds of start and end)

@jrmylow
Copy link
Contributor Author

jrmylow commented Nov 25, 2023

pre-commit.ci autofix

@MarcoGorelli MarcoGorelli self-requested a review November 27, 2023 10:18
Copy link
Contributor

This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this.

@github-actions github-actions bot added the Stale label Dec 28, 2023
@jrmylow
Copy link
Contributor Author

jrmylow commented Dec 29, 2023

This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this.

updated and still pending review

@MarcoGorelli
Copy link
Member

sorry for the delay, will try to get to it this week or next

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Sorry this has taken me a while

First, can we move this to the 2.3.0 whatsnew note please?

Second, is it possible to fix the two bugs independently?
As in, I think you can just remove the

    elif end and not offset.is_on_offset(end):
        # Incompatible types in assignment (expression has type "datetime",
        # variable has type "Optional[Timestamp]")
        end = offset.rollback(end)  # type: ignore[assignment]

completely, and all existing tests will pass, as well the snippet from 56134?

If so, I'd suggest splitting this - first, remove the unnecessary elif end block, and then we fix the if start and not ... one separately

@jrmylow jrmylow closed this Jan 11, 2024
@jrmylow jrmylow deleted the 2023-11-date_range-fix branch January 11, 2024 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants