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 'date' as alias to 'day' for startOf() and endOf(). #2982

Closed
wants to merge 1 commit into
base: develop
from

Conversation

Projects
None yet
5 participants
@datyayu
Contributor

datyayu commented Feb 23, 2016

Patch for #2974.

@datyayu

This comment has been minimized.

Contributor

datyayu commented Feb 23, 2016

@icambron Sure, makes sense. Updated.

@icambron

This comment has been minimized.

Member

icambron commented Feb 23, 2016

LGTM, @ichernev

@mj1856

This comment has been minimized.

Member

mj1856 commented Feb 24, 2016

I don't understand why the if statement is needed at all in the endOf function. Shouldn't it just be handled by the case statement in the startOf function?

For that matter, looking closer here, I don't understand why in the existing code there's an inline if for treating an isoWeek as a locale-based week, or why endOf is shortcutting for undefined and millisecond but startOf doesn't.

IMHO, all of the logic should be in the startOf function. The endOf function should be consistent in going to the start of the next unit and then back one millisecond.

@datyayu

This comment has been minimized.

Contributor

datyayu commented Mar 4, 2016

Sorry for the delay, I've been busy lately.

The if statement sets the units to a valid value for the add method. Maybe the inline isoWeek handles the same case.

As for the units === undefined I think it isn't on startOf because startOf always returns this and ignores unhandled cases, but endOf returns by default a whole method chain, which is why it returns this on the undefined case, to avoid having to perform the operations with a wrong value. I'm not really sure about why the handling for the milliseconds..

@maggiepint

This comment has been minimized.

Member

maggiepint commented Mar 25, 2016

I want to keep this pull request moving along, so:

I agree that undefined shortcutting is correct because of the return of the whole method chain.

Yes, date needs to be changed to day in endOf to make the add call valid.

This actually opens up a bit of a rabbit hole. Ideally, by this logic, date would be an alias for day in the add/subtract functions. And that code path will lead you down to making date and alias for day in durations. I am not sure if this spirals out further, and it's late enough at night that I don't want to find out right now.

If we don't want to go make the day/date aliasing thing work everywhere, I think this PR is generally fine. I'm open to the opinions of others on whether we should carry the aliasing through. Programatically it makes sense to carry it all the way through, but from an API design standpoint it's a little wonky. Why would one add a date? One adds a day.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Apr 11, 2016

We'll merge this, the code is not perfect, but it works :)

@ichernev

This comment has been minimized.

Contributor

ichernev commented Apr 16, 2016

Merged in 5393485

@ichernev ichernev closed this Apr 16, 2016

ichernev added a commit that referenced this pull request Apr 16, 2016

Merge pull request #2982 from datyayu:develop
Add 'date' as alias to 'day' for startOf() and endOf().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment