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/durations #265

Closed
wants to merge 6 commits into from
Closed

Feature/durations #265

wants to merge 6 commits into from

Conversation

rockymeza
Copy link
Contributor

This is a new feature based on the discussion in #236.

I created a new object called Duration that can be added and subtracted. It can be instantiated using the moment.duration function. There are three ways to instantiate a Duration object: Object, key/value, and millisecond. The following five invocations result in essentially the same Duration object.

moment.duration({seconds: 45});
moment.duration({s: 45});
moment.duration(45, 'seconds');
moment.duration(45, 's');
moment.duration(45000);

I exposed the Duration.prototype on moment.duration.fn. On it currently there are two methods: toValue and humanize. toValue returns the milliseconds of all the options added together. humanize contains the functionality that moment.humanizeDuration used to contain.

I deprecated moment.humanizeDuration, but I left it and the tests in place. It now relies on Duration.prototype.humanize. moment.fn.from also relies on the Duration.prototype.humanize now instead of humanizeDuration.

Things that it doesn't do (yet)

  • There is no isDuration function to type-check Duration objects.
  • When adding using a duration, it basically clones the duration object by calling moment.duration on it. So, there is no special handling of Durations versus normal Objects in adding and subtracting.
  • If you create a Duration of 7 days and check the weeks property, it will still be zero.

Don't merge this in yet.

I am opening this pull request so that we can continue the conversation and also start developing this feature.

cc @nilakanta

Rocky Meza added 2 commits April 7, 2012 09:05
moment.duration provides an instantiation method for Duration objects
that handles three input types: an Object, key/value, or just
milliseconds.  I moved the functionality of moment.humanizeDuration to
moment.duration.fn.humanize.  I left a deprecated
moment.humanizeDuration method which relies on moment.duration.
moment.fn.from has been updated to use moment.duration.
@timrwood
Copy link
Member

timrwood commented Apr 8, 2012

This looks really good! Much cleaner than the old object literals. All the api stuff looks good.

What do you think of storing the internal data as milliseconds, days, and months? Then we can deduce hours/min/sec from milliseconds, weeks from days, and years from months like in dateAddRemove?

@nilakanta
Copy link

@rockymeza Looks great.
@timrwood Storing internal data as just milliseconds would be good, while keeping the object literals intact. That would make duration arithmetic a easier.

@ghost ghost assigned rockymeza Apr 9, 2012
@rockymeza
Copy link
Contributor Author

If we store information in milliseconds, days, and months, maybe we could have getters for the other formats.

var duration = moment.duration(60000); // sixty thousand milliseconds

moment.milliseconds(); // 60000 milliseconds
moment.seconds(); // 60 seconds
moment.minutes(); // 1 minute

Should there be rounding?

@nilakanta, I thought about storing everything as milliseconds, but that doesn't work out. For example:

var feb23 = moment([2012, 1, 23])
  , mar23 = moment([2012, 2, 23])
  , apr23 = moment([2012, 3, 23]);

feb23.diff(mar23, 'months') == mar23.diff(apr23, 'months'); // true
feb23.diff(mar23, 'milliseconds') == mar23.diff(apr23, 'milliseconds'); // false

@nilakanta
Copy link

@rockymeza Thanks for the explanation. Then it makes sense to store in milliseconds, days, and months.

@icambron
Copy link
Member

@rockymeza - I like this a lot. On rounding, I think yes. Otherwise:

var duration = moment.duration(60001);
duration.seconds();  //60.001

Seems pretty silly.

@rockymeza
Copy link
Contributor Author

What should the getters return? For example,

var duration = moment.duration({seconds: 1});
duration.milliseconds(); // 1000, makes sense

var duration = moment.duration({months: 1});
duration.years(); // 0, makes sense
duration.months(); // 1, makes sense
duration.days(); // could be 28, 29, 30, or 31
duration.weeks(); // ???

@timrwood
Copy link
Member

There are two different cases that I can think of for getting these values.

  1. Get the length of the duration in these units. (1.5 hours = 90 minutes)
  2. Get the length of the duration in this unit minus the length of the parent unit. (1.5 hours = 1 hour 30 minutes)

Number 2 is essentially how the getters work on moments. I can also see the use case of number 1 though, even though it doesn't really apply to moments.

What about something like this?

var duration = moment.duration({months:1, days:20, hours:10, minutes:5});
duration.months(); // 1
duration.days(); // 20
duration.hours(); // 10
duration.minutes(); // 5

duration.asMonths(); // 1.67...
duration.asDays(); // 50.46...
duration.asHours(); // 1210.12...
duration.asMinutes(); // 72612

The months days hours would clip to their min/max ranges (0 - 12, 0 - 30, 0 - 24), and the asMonths asHours asMinutes would include all three units (milliseconds, days, and months).

Rocky Meza added 4 commits April 15, 2012 00:20
These can serve as encapsulation to protect from future possible changes
in internal representation.
This is according to the discussion in #265.
@rockymeza
Copy link
Contributor Author

I added both sets of getters, do you guys think that they have the correct functionality?

The as getters, asYear, etc. do not do any rounding, which might be annoying, considering rounding to a specific decimal point is awkward in JavaScript:

Math.round(1234.1234 * 100) / 100; // 1234.12

Here is an example of the behavior provided by the other getters too. Does it make sense?

var d = moment.duration({minutes: 90});
d.hours(); // 1
d.minutes(); // 30

var d2 = moment.duration({hours: 25, minutes: 90});
d2.days(); // 1
d2.hours(); // 2
d2.minutes(); // 30

I too am not sure of a use case for this functionality, so I don't know what the behavior should be.

@timrwood
Copy link
Member

Sorry I've been awol for a while. I just got a chance to take a look at the code for this, and I really like it all.

I too am not sure of a use case for this functionality, so I don't know what the behavior should be.

The use case I was thinking of was something like #264 or #192/#143. Basically anything that needs the difference split up into different units.

Regarding the asYear getters, maybe we could do something like moment.fn.diff and add a parameter to return the float or rounded number.

Also, I added an issue #282 for potentially switching to returning a duration in moment.fn.diff, please check it out and share your opinion.

@timrwood
Copy link
Member

I'm merging this into develop, thanks for working on it Rocky!

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.

None yet

4 participants