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 bug in FiniteDateRange.days property #98

Merged
merged 1 commit into from
Oct 17, 2023
Merged

Conversation

josh-lynch
Copy link
Contributor

Previously this property would return a value exclusive of the end
date. However the FiniteDateRange defines it's boundaries as double
inclusive, therefore this value was off by one.

This change includes the range end date in the count of days.

Previously this property would return a value exclusive of the end
date. However the FiniteDateRange defines it's boundaries as double
inclusive, therefore this value was off by one.

This change includes the range end date in the count of days.
@josh-lynch
Copy link
Contributor Author

This PR addresses a long standing bug in the ranges module. It seems likely that a lot of code will have been developed to depend on this bug, as such we will need to be careful about the release of this change.

For kraken-core (and other in house repos I assume) we can rely on CI picking up any potential breakages this change will cause. However as this repo is now open source I'm not sure how we manage the risk for other users, do we need to consider a major version change?

@josh-lynch
Copy link
Contributor Author

Related PR: #99

@josh-lynch josh-lynch requested a review from a team October 12, 2023 11:36
xocto/ranges.py Show resolved Hide resolved
Copy link
Contributor

@samueljsb samueljsb left a comment

Choose a reason for hiding this comment

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

I think this is an obvious bug with an obvious fix 👍🏻

Copy link

@leamingrad leamingrad left a comment

Choose a reason for hiding this comment

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

LGTM!

@josh-lynch josh-lynch merged commit 94fff47 into main Oct 17, 2023
1 check passed
@josh-lynch josh-lynch deleted the ranges/bugfix-days branch October 17, 2023 12:20
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.

4 participants