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

fea: add isInRange extension #76

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

guzishiwo
Copy link

 Returns true if this DateTime is in the range [start]-[end].
  [startInclusive] and [endInclusive] control the range boundaries:
- Left-open right-open (false, false): (start, end)
- Left-open right-closed (false, true): (start, end]
- Left-closed right-open (true, false): [start, end)
- Left-closed right-closed (true, true): [start, end]

@FMorschel
Copy link
Collaborator

FMorschel commented Dec 29, 2024

Hello, @guzishiwo! Thanks for your PR!

It seems fine to me. I'd simply ask you to make more tests (for true and false for all cases of inclusive and exclusive). It can be rightfully implemented but we should be able to maintain it this way (if we ever need to refactor something) with more tests.

I'd also suggest you take a look at https://pub.dev/packages/due_date (https://github.com/FMorschel/due_date) which has a Period class that has this kind of tests (and some generators that take full days into account) and if you're willing, we can think of a wrapper class (on that package) that takes a Period and the booleans you have so we can test them in a similar way (maybe add an extension to facilitate the use).

I'm not sure what @jogboms thinks about having this on time, it seems a simple enough implementation for me. I'm not sure I'd personally do it with compareTo. If I did, I'd probably rename left and right to something more meaningful if I could think of anything, but it is hard since it returns an integer.

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.

2 participants