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

Calculating "last Wednesday", etc. #2522

Closed
mattjohnsonpint opened this issue Jul 29, 2015 · 13 comments
Closed

Calculating "last Wednesday", etc. #2522

mattjohnsonpint opened this issue Jul 29, 2015 · 13 comments

Comments

@mattjohnsonpint
Copy link
Contributor

Reading the docs for the day setter, I see these examples:

moment().day(-7); // last Sunday (0 - 7)
moment().day(7); // next Sunday (0 + 7)
moment().day(10); // next Wednesday (3 + 7)
moment().day(24); // 3 Wednesdays from now (3 + 7 + 7 + 7)

These don't necessarily work consistently. For example, consider:

moment('2015-07-29').day(6-7).format("YYYY-MM-DD (dddd)"); // "2015-07-25 (Saturday)"
moment('2015-07-29').day(0-7).format("YYYY-MM-DD (dddd)"); // "2015-07-19 (Sunday)"

I asked for "last Saturday", and "last Sunday" respectively. Since the supplied date is a Wednesday, I would expect the results to only be a day apart (the 25th and 26th).

Consider also that we support .day("Sunday") in string format. Both numeric and string forms would appear to prefer the current week - which is fine. It's just the bubbling function of the numeric form that doesn't make logical sense.

I propose:

  • Remove the bubbling function. Either modulo to the same day of week, or throw an error when out of range.
  • Add some new functions for getting the last/next instance of a day-of-week, perhaps with an optional n number of weeks (defaulting to 1).
  • Do the same thing with respect to locale-aware day of week functions and ISO day of week functions.
  • Update the docs accordingly.
@ichernev
Copy link
Contributor

I think the implementation makes sense -- we can imagine today being the Xth day of the week, and you request to set Y (might be negative/above 7). So we take the offset and move that many days up/down.

It depends on what is the begining of the week. If -7 means the Sunday of last week, then 0 means Sunday of this week, then week starts on Sunday. And if the week starts on Sunday (0), then this will be not the previous but the one before that.

So actually, if you want previous Sunday, you should do 7 - 7, and get 0, but note that 7 is Sunday for you, if the week starts at Monday, so its fine. You counted week start in one system, and expect the answer to be in another.

Also are your proposed functions based on actual need to compute something? You can take the beginning of the week with a locale and do +/- 7 * n + m. I don't think its that common to require specialization.

@mattjohnsonpint
Copy link
Contributor Author

@ichernev - respectfully, I disagree. If I want to determine "last Tuesday", it only matters the day of the week I'm starting from. It does not matter at all which day I choose to start my week.

The current function only allows one to choose a day "within the current week", or "within the previous week", or "within the next week". That's quite different than saying "this Tuesday", "last Tuesday", or "next Tuesday".

@ichernev
Copy link
Contributor

Well, you're suggesting a change in semantics of the "last" and "next" words. To be fair

  1. I'm pretty sure the meaning is different in different locales
  2. People interpret them differently, in any case

If today is Monday, and somebody says next Tuesday, I'd assume the Tuesday of next week, but if it is Wednesday, I won't know if it was next week's Tuesday, or the one after that. If we use your semantics it should be next-next Tuesday (the one that is 2 weeks from now).

We've had this discussion at least 3 times before. I can only agree on adding methods that handle both and let the users choose. Maybe in the future add locale option to specify the "default" for a locale, but that is a long stretch already.

@mattjohnsonpint
Copy link
Contributor Author

All I'm saying is we don't have a reliable way to get the immediate-next or immediate-prior date matching the day-of-week.

How the week falls onto the calendar in the specific locale should not be relevant to this.

@mattjohnsonpint
Copy link
Contributor Author

@vaibhavi3t
Copy link

I wanted to work on this. Please guide me how to proceed. What should i know to figure out this issue?

@mslooten
Copy link

I think that the current implementation of .day() is as expected (it's anchored within the current week, right?). But is there a need for a new method to get the last and the next day? I can imagine something like this:

So if we take the current date (2016-08-27):

// .lastDay(day, offset)
moment().lastDay("Friday").format("YYYY-MM-DD"); // "2016-08-26"
moment().lastDay("Friday", 1).format("YYYY-MM-DD"); // "2016-08-19" (offset 1 wk)

// .nextDay(day, offset)
moment().nextDay("Wednesday").format("YYYY-MM-DD"); // "2016-08-31"
moment().nextDay(3).format("YYYY-MM-DD"); // "2016-08-31"
moment().nextDay().format("YYYY-MM-DD"); // "2016-09-03"

So two similar methods that take two arguments, the day (defaults to current day one week from now) and the offset in weeks (defaults to 0).

If there's a place and need for this, I'd like to jump in create a PR for it – but I'm not sure if it's within the scope of moment.js.

@mslooten
Copy link

@mj1856, is there still a need for this?

@maggiepint
Copy link
Member

@mslooten I kinda like that API, but something about the wording seems a little of. To me, moment().nextDay() should give me tomorrow. I'm trying to think of a better thing to call it. Possibly just .next() and .last().

I think we would take this pull if the naming were cleaned up. Any ideas @mj1856 @ichernev @icambron @butterflyhug ?

@butterflyhug
Copy link
Contributor

I think that API would be a sensible feature addition. I like the name .next(), and I think something like .previous() would be a clearer name than .last().

@nikshepsvn
Copy link

is this still open? would love to give this a shot! can someone give me an idea of how to start off?

@bravodavid56
Copy link

I'm not sure if we're referring to the same problem, but I use this solution for getting the date for a specific day of the week. It involves using moment().startOf('week')
This will give you a Moment object for the Sunday that just passed. If you want the Sunday before that, you can subtract a week, and if you want the Sunday after that, you can add a week.

In this case, getting last Wednesday, if you mean Wednesday of last week, then you can

  1. subtract a week from today
  2. get the start of that week (this would give you the Sunday of that week)
  3. add the appropriate amount of days for Sunday to Wednesday (Wednesday is 3 days away from Sunday, so +3 days)

Getting next Wednesday, (or Wednesday of next week), you can

  1. add a week from today
  2. get the start of that week (this would give you the Sunday of that week)
  3. add the appropriate amount of days for Sunday to Wednesday (repeating step 3 from above)

@marwahaha
Copy link
Member

See https://momentjs.com/docs/#/-project-status/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants