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

Feature/implement generic add #1581

Merged
merged 19 commits into from Jan 4, 2020

Conversation

Anshuman71
Copy link
Contributor

Implement the add(Object) function from Moment and fix #1427

Add 2 hours, 15 minutes and 30 seconds to 10 July 2014 12:45:00;

var result = add(new Date(2014, 6, 10, 12, 45, 0), { minutes: 15, hours: 2, seconds: 30 });
result: Thu Jul 10 2014 15:00:30

Supports adding

{
hours: number,
years: number,
days: number,
quarters: number,
weeks: number,
months: number,
minutes: number,
seconds: number,
milliseconds: number
}
combined in a single call

@kossnocorp
Copy link
Member

@Anshuman71 thank you a lot!

@Anshuman71
Copy link
Contributor Author

@Anshuman71 thank you a lot!

Can you please help me with this failing test?

@kossnocorp
Copy link
Member

@Anshuman71 sure, I'm working on a review atm, and tests will pass once you address my comments. Please stay put.

Copy link
Member

@kossnocorp kossnocorp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great and very important work, thank you so much! Please address my comments before we proceed. I didn't check the date math yet, I will do with the second review round.

src/add/index.js Outdated Show resolved Hide resolved
src/add/index.js Outdated Show resolved Hide resolved
src/add/index.js Outdated Show resolved Hide resolved
src/add/index.js Outdated Show resolved Hide resolved
src/add/index.js Outdated Show resolved Hide resolved
src/add/index.js Outdated Show resolved Hide resolved
src/add/index.js Outdated Show resolved Hide resolved
src/add/index.js Outdated Show resolved Hide resolved
@kossnocorp
Copy link
Member

@Anshuman71 I'm going to push some changes in the PR. I reevaluated how other functions handle incorrect arguments, and now I think RangeError is a wrong approach. I also want to adjust tests a bit. Please don't push anything.

@Anshuman71
Copy link
Contributor Author

@Anshuman71 I'm going to push some changes in the PR. I reevaluated how other functions handle incorrect arguments, and now I think RangeError is a wrong approach. I also want to adjust tests a bit. Please don't push anything.

Sure, go ahead

@kossnocorp kossnocorp force-pushed the feature/implement-generic-add branch from 244cadc to b4b95dd Compare January 4, 2020 07:51
- Return Invalid Date if the duration is invalid
- Get rid of weeks and milliseconds not present in ISO 8601's duration
- Use addMonths
- Cleanup code
- Cleanup docs
@kossnocorp
Copy link
Member

I've pushed some changes. Most notable changes are:

  • I've rolled back the Invalid Error on invalid duration. You were right returning it.
  • I've removed weeks and milliseconds to make the duration object correspond to ISO 8601's duration. This will help us down the road when we are going to add support to it the library.
  • I reused addMonths to avoid copy-paste of the code with quirks.

Copy link
Member

@kossnocorp kossnocorp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

@Anshuman71
Copy link
Contributor Author

Great work!

@kossnocorp thanks for your help in shipping the feature

@kossnocorp
Copy link
Member

You're welcome!

@kossnocorp kossnocorp merged commit 6266fa2 into date-fns:master Jan 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feat: multi add function to add multiple parts of a date
2 participants