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

adding previousDay and its variations #2522

Merged
merged 8 commits into from Sep 16, 2021

Conversation

@LucasHFS
Copy link
Contributor

@LucasHFS LucasHFS commented Jun 18, 2021

[WIP] Implements #2507 request, and similar functions.

@LucasHFS LucasHFS force-pushed the 2507_add_previoursMonday branch 4 times, most recently from 7fde6ae to b85fae6 Jun 18, 2021
@tan75 tan75 linked an issue that may be closed by this pull request Jun 18, 2021
@tan75 tan75 self-assigned this Jun 18, 2021
@LucasHFS LucasHFS force-pushed the 2507_add_previoursMonday branch 2 times, most recently from 7ef7d9b to 506e78f Jun 18, 2021
@tan75
Copy link
Contributor

@tan75 tan75 commented Jun 18, 2021

Hi @LucasHFS
thanks a mill!
I linked the mentioned issue to this PR and marked it as WIP until it gets approval.
Can you please have a look into this test as it is failing?

  41 |   it('returns the previous Tuesday given the Saturday after it', function () {
    > 42 |     assert.deepStrictEqual(

if you prefer to run tests locally you can do
yarn test

Many thanks!

@fturmel
Copy link
Contributor

@fturmel fturmel commented Jun 18, 2021

Minor stuff, but you have a little typo in your tests (previours instead of previous).

Could I also propose a simpler and faster (by about 50%) implementation for previousDay? nextDay could be refactored using the same approach as well.

export default function previousDay(dirtyDate: Date | number, day: Day): Date {
  requiredArgs(2, arguments)

  const date = toDate(dirtyDate)

  let delta = getDay(date) - day
  if (delta <= 0) delta += 7

  return subDays(date, delta)
}

@LucasHFS LucasHFS changed the title adding previoursDay and its variations adding previousDay and its variations Jun 20, 2021
@LucasHFS
Copy link
Contributor Author

@LucasHFS LucasHFS commented Jun 20, 2021

Sure, tomorrow I'll work on this.

@fturmel
Copy link
Contributor

@fturmel fturmel commented Jun 21, 2021

@LucasHFS @tan75

There's already a PR for the nextDay optimization that has been approved (#2524). Should the change be reverted here or should I close 2524?

@tan75
Copy link
Contributor

@tan75 tan75 commented Jun 22, 2021

@LucasHFS @tan75

There's already a PR for the nextDay optimization that has been approved (#2524). Should the change be reverted here or should I close 2524?

This PR 2524 will be included in our next release hopefully soon enough.
And #2522 can be added after. If any conflicts - we will fix it!

Thanks guys! 👍 🥇

@tan75 tan75 force-pushed the 2507_add_previoursMonday branch from e10c1a8 to 8de1be7 Jul 30, 2021
tan75
tan75 approved these changes Jul 30, 2021
Copy link
Contributor

@tan75 tan75 left a comment

Thanks a lot for your contribution! 🚀

@fturmel
Copy link
Contributor

@fturmel fturmel commented Jul 30, 2021

Just had a look again at this PR and I think there's no need to convert the date argument using toDate before passing it to previousDay. This optimization applies to all the next* functions as well (nextMonday, etc).

ex:

export default function previousThursday(date: Date | number): Date {
  requiredArgs(1, arguments)
  return previousDay(toDate(date), 4)
}

could simply be:

export default function previousThursday(date: Date | number): Date {
  requiredArgs(1, arguments)
  return previousDay(date, 4)
}

@tan75
Copy link
Contributor

@tan75 tan75 commented Jul 30, 2021

@fturmel this totally makes sense! Thank you!
Will need to change the next* ones in a separate PR.

src/previousSunday/index.ts Outdated Show resolved Hide resolved
src/previousSaturday/index.ts Outdated Show resolved Hide resolved
src/previousDay/index.ts Outdated Show resolved Hide resolved
@kossnocorp kossnocorp merged commit 0e5d948 into date-fns:master Sep 16, 2021
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants