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.

Sign up for GitHub

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

Jump to bottom

Implement contains, intersection and of methods for Interval #64

Merged
merged 3 commits into from
May 19, 2023
Merged

Implement contains, intersection and of methods for Interval #64

merged 3 commits into from
May 19, 2023

Conversation

someniatko
Copy link
Contributor

closes #63

@someniatko
Copy link
Contributor Author

Do you think it makes sense to also implement isEqualTo() and toNativeDatePeriod()?

@someniatko someniatko changed the title Implement contains, intersection and of methods for Instant Implement contains, intersection and of methods for Interval Aug 15, 2022
@someniatko
Copy link
Contributor Author

@BenMorel ping 🤸

BenMorel
BenMorel requested changes May 17, 2023
View reviewed changes
Copy link
Member

@BenMorel BenMorel left a comment

Choose a reason for hiding this comment

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

A couple changes, otherwise looks good to me 👍

BenMorel

This comment was marked as duplicate.

Sign in to view

@BenMorel
Copy link
Member

Do you think it makes sense to also implement isEqualTo() and toNativeDatePeriod()?

  • isEqualTo(): yes, and you may use it in your tests as per my comment
  • toNativeDatePeriod(): it looks like DatePeriod is about recurring periods, so I don't think they represent the same concept. I'd pass on this one.

@someniatko
Copy link
Contributor Author

someniatko commented May 18, 2023 •
edited
Loading

Okay, I will implement isEqualTo() as well.

@someniatko someniatko force-pushed the interval-enhancements branch from f31225a to 41cfbe2 Compare May 18, 2023 08:28
@someniatko someniatko force-pushed the interval-enhancements branch 2 times, most recently from 05976ba to b44c4c1 Compare May 18, 2023 08:35
@someniatko
Copy link
Contributor Author

Done! Code coverage has been decreased for the classes this PR is not responsible for, maybe something has changed in the method of the coverage calculation.

@someniatko someniatko requested a review from BenMorel May 18, 2023 09:05
BenMorel
BenMorel requested changes May 19, 2023
View reviewed changes
@someniatko someniatko force-pushed the interval-enhancements branch from b44c4c1 to a975807 Compare May 19, 2023 20:47
@someniatko someniatko requested a review from BenMorel May 19, 2023 20:52
@BenMorel BenMorel merged commit 8739cbb into brick:master May 19, 2023
@BenMorel
Copy link
Member

Thank you! Released as 0.4.2 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenMorel BenMorel BenMorel approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Interval is so barebones compared to LocalDateRange
2 participants
@someniatko @BenMorel