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

Add lastMoment getter to Interval #1280

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

ties-s
Copy link

@ties-s ties-s commented Sep 3, 2022

Convenient way to get the last moment that is actually included in the interval. Makes it easier to properly use the fact that intervals are half open.

My use case: displaying end date of a monthly subscription.

I'm not sure lastMoment is the best way to name this, but it was the first thing that I thought of.

Convenient way to get the last moment that is actually included in the interval. Makes it easier to properly use the fact that intervals are half open.

My use case: displaying end date of a monthly subscription.

I'm not sure `lastMoment` is the best way to name this, but it was the first thing that I thought of.
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 3, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@icambron
Copy link
Member

icambron commented Mar 4, 2023

@ties-s So first, sorry for taking so long to get to this.

I'm open to this but a couple of notes:

  1. I think lastMillisecond() is a better name - the term "moment" is not defined in Luxon's documentation or API
  2. It needs test, as simple as those tests would be.

@ties-s
Copy link
Author

ties-s commented Mar 4, 2023

@icambron thank you for your response.

I'll look into the tests.

For the name, lastMillisecond might suggest a numeric return value. Some other options I thought of, but are also not great:

  • inclusiveEnd
  • endInclusive
  • lastInstant

@icambron
Copy link
Member

icambron commented Aug 9, 2023

@ties-s sorry a second time for the slow review here. I agree with your concern that lastMillisecond sounds numeric. How about lastDateTime

@ties-s
Copy link
Author

ties-s commented Aug 18, 2023

@icambron That sounds good, updated the PR to reflect that new function name

@diesieben07
Copy link
Collaborator

@ties-s Looks good to me! Once we have 3.4.2 out with a fix for the current Duration issues, this can be merged.

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.

None yet

3 participants