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 gain validity #3611

Closed
wants to merge 11 commits into
base: develop
from

Conversation

Projects
None yet
5 participants
@marwahaha
Member

marwahaha commented Nov 19, 2016

This was hoping to resolve #3225

  • Creates an _isValid property for durations, and an accessor isValid()
  • If not valid, create a duration of time 0 and set ._isValid to false
  • .humanize() will return the translated version of Invalid Date if not valid
  • duration(null), duration(undefined), duration() will all be valid
  • duration(NaN) is invalid
@jsf-clabot

This comment has been minimized.

jsf-clabot commented Nov 19, 2016

CLA assistant check
All committers have signed the CLA.

@@ -53,10 +53,17 @@ test('milliseconds instantiation', function (assert) {
test('undefined instantiation', function (assert) {
assert.equal(moment.duration(undefined).milliseconds(), 0, 'milliseconds');
assert.equal(moment.duration(undefined)._isValid, true, '_isValid');

This comment has been minimized.

@icambron

icambron Nov 25, 2016

Member

This needs an isValid() accessor function. We don't want people checking underscored properties.

@@ -53,10 +53,17 @@ test('milliseconds instantiation', function (assert) {
test('undefined instantiation', function (assert) {
assert.equal(moment.duration(undefined).milliseconds(), 0, 'milliseconds');
assert.equal(moment.duration(undefined)._isValid, true, '_isValid');
});
test('null instantiation', function (assert) {
assert.equal(moment.duration(null).milliseconds(), 0, 'milliseconds');

This comment has been minimized.

@icambron

icambron Nov 25, 2016

Member

This doesn't really fix the #3225 because humanize will still output "in a few seconds".

@marwahaha

This comment has been minimized.

Member

marwahaha commented Nov 29, 2016

@icambron let me know what you think! I've fixed as per your review.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Dec 4, 2016

@marwahaha this is a good starting point. But to make it really work isValid needs to be viral. What I mean is that if a duration is invalid most operations should return invalid durations/values/whatever. Here's a list of all the prototype functions and what they should return. This should be implemented and tested (check src/test/moment/invalid.js).

abs -> invalid
add -> invalid
subtract -> invalid
as -> NaN
asMilliseconds -> NaN
asSeconds -> NaN
asMinutes -> NaN
asHours -> NaN
asDays -> NaN
asWeeks -> NaN
asMonths -> NaN
asYears -> NaN
valueOf -> NaN
get -> NaN
milliseconds -> NaN
seconds -> NaN
minutes -> NaN
hours -> NaN
days -> NaN
weeks -> NaN
months -> NaN
years -> NaN
humanize -> invalid duration localized string
toISOString -> invalid duration localized string
toString -> invalid duration localized string
toJSON -> null
locale -> no change in behavior
localeData -> no change in behavior
@@ -50,3 +54,8 @@ export function toISOString() {
(m ? m + 'M' : '') +
(s ? s + 'S' : '');
}
export function toJSON() {
// this may not be fully implemented

This comment has been minimized.

@marwahaha

marwahaha Dec 5, 2016

Member

For some reason, .toJSON was previously mapped to .toISOString. I've created a separate function, at least, to handle the difference when the duration is invalid.

This comment has been minimized.

@ichernev

ichernev Feb 4, 2017

Contributor

@maggiepint answered your question -- just patch toISOString and leave toJSON an alias to it.

@marwahaha

This comment has been minimized.

Member

marwahaha commented Dec 5, 2016

Ok, I've done this @ichernev

Two nits:
the toJSON method does not look implemented. The scope of that deserves a different PR.
So far, this isValid checker is quite lax about actually creating invalid durations. One of the only "invalid" durations I could make was just moment.duration(NaN). Should durations be invalid in other cases?

@marwahaha

This comment has been minimized.

Member

marwahaha commented Dec 20, 2016

How does this look?

@maggiepint

This comment has been minimized.

Member

maggiepint commented Jan 8, 2017

Okay, circling back to this. This is good work, thank you.

What other cases do you expect to handle with toJSON()? I'm not sure what you mean by not implemented. When you pass a duration inside of a JSON object, ISO format works just fine, which is why it delegates to .toIsoString. I agree that passing null if it is invalid makes sense, but I'm not sure what else there would need to be.

As for what constitutes an invalid duration - that one might best be handled by @mj1856 who is probably the most interested in standards, though I'm pretty sure @butterflyhug is pretty darn good at durations too from his old gig. Based on the Wikipedia interpretation of the standard though, I believe that any duration with a decimal on a unit other than the smallest unit is technically invalid.

@marwahaha

This comment has been minimized.

Member

marwahaha commented Jan 9, 2017

Ok @maggiepint !

  1. I'm fine with leaving the toJSON() as is. Your explanation makes sense.

  2. How should I move forward here? I suppose I could check whether numbers other than the smallest are integers, but it might be too restrictive. (Should we allow 5.1 days and 2 milliseconds?)

@marwahaha

This comment has been minimized.

Member

marwahaha commented Jan 10, 2017

Let me know how I can help here @mj1856 @butterflyhug

@marwahaha

This comment has been minimized.

Member

marwahaha commented Jan 18, 2017

Pinging this thread again @mj1856 @maggiepint @butterflyhug

@maggiepint

This comment has been minimized.

Member

maggiepint commented Jan 18, 2017

What you are saying is correct. Numbers other than the smallest should be integers according to the spec.

@marwahaha

This comment has been minimized.

Member

marwahaha commented Jan 21, 2017

Ok this is updated! @maggiepint I did change two tests because they used an invalid duration (-0.5 years, 1 month).

Let me know if I can help further.

@marwahaha

This comment has been minimized.

Member

marwahaha commented Jan 31, 2017

Bumping this thread.

@ichernev

The code and tests look good, just a few small tweaks and its ready to go.

As I said in one of the comments, we can't go too harsh on bad input, because we were happily accepting it in the previous release, and its not really obvious what is right anymore: duration({d: null}) -- so is this invalid, or should it be treated as 0? I'd say keep it rolling for now and rething in v3.

var normalizedInput = normalizeObjectUnits(duration);
this._isValid = isDurationValid(normalizedInput);
this.isValid = function () {

This comment has been minimized.

@ichernev

ichernev Feb 4, 2017

Contributor

isValid should be a regular prototype function, not something attached to this on every object creation.

@@ -50,3 +54,8 @@ export function toISOString() {
(m ? m + 'M' : '') +
(s ? s + 'S' : '');
}
export function toJSON() {
// this may not be fully implemented

This comment has been minimized.

@ichernev

ichernev Feb 4, 2017

Contributor

@maggiepint answered your question -- just patch toISOString and leave toJSON an alias to it.

@@ -8,7 +8,7 @@ import { as, asMilliseconds, asSeconds, asMinutes, asHours, asDays, asWeeks, asM
import { bubble } from './bubble';
import { get, milliseconds, seconds, minutes, hours, days, months, years, weeks } from './get';
import { humanize } from './humanize';
import { toISOString } from './iso-string';
import { toISOString, toJSON } from './iso-string';

This comment has been minimized.

@ichernev

ichernev Feb 4, 2017

Contributor

So no changes in this file whatsoever.

export default function isDurationValid(m) {
for (var key in m) {
if (ordering.indexOf(key) === -1 ||
m[key] !== undefined && isNaN(parseInt(m[key]))) {

This comment has been minimized.

@ichernev

ichernev Feb 4, 2017

Contributor

I'm amazed jscs doesn't catch this but this line needs to be indented 8 more spaces (continuation indent).

This comment has been minimized.

@ichernev

ichernev Feb 4, 2017

Contributor

Rewrite to:
if (!(ordering.indexOf(key) !== -1 && (m[key] == null || !isNaN(m[key]))))
The tricky part is -- I want to explicitly allow both null and undefined (they'll be treated as 0 because of the || 0 code later). Also you don't need parseInt when doing isNaN (its built in).

I hope we can have some stricter rules in a future major release, but right now even this is getting dangerous (as it will break someone's code).

return this._isValid;
};
var years = this._isValid && normalizedInput.year || 0,

This comment has been minimized.

@ichernev

ichernev Feb 4, 2017

Contributor

We have a one var policy in jshint, so this won't fly. Just don't change the code (leave it as it was) -- if its invalid its safe to exit this function. All other functions check for isValid, so its fine if the internals are messed up.

if (unitHasDecimal) {
return false; // only allow non-integers for smallest unit
}
if (parseFloat(m[ordering[i]]) !== parseInt(m[ordering[i]])) {

This comment has been minimized.

@ichernev

ichernev Feb 4, 2017

Contributor

Use toInt functions from the src/lib/utils/to-int.js instead of parseInt.

});
test('NaN instantiation', function (assert) {
assert.ok(isNaN(moment.duration(NaN).milliseconds()), 'milliseconds should be NaN');

This comment has been minimized.

@ichernev

ichernev Feb 4, 2017

Contributor

That is good. For tests it makes sense to add a helper method moment.duration.invalid(), the same way we have for moment, so you can properly construct an invalid duration.

for (i = 0; i < invalids.length; ++i) {
invalid = invalids[i];
assert.ok(!invalid.add(5, 'hours').isValid(), 'invalid.add is invalid');

This comment has been minimized.

@ichernev

ichernev Feb 4, 2017

Contributor

For loops like this -- include the i into the string description (so when it fails you can debug it), or scrap the loop altogether -- this is unit test, you check first what is invalid, and then you check what invalid means, you don't have to test end-to-end.

@marwahaha

This comment has been minimized.

Member

marwahaha commented Feb 6, 2017

Ok @ichernev !

  • I added a test for moment.duration({d: null}).
  • Should I add a helper method moment.duration.invalid()? I couldn't find where moment.invalid() was implemented.
  • this.isValid() does not re-compute the value of this._isValid. If it's not ok, I can change it.
  • All other requests are completed.

@ichernev ichernev added this to the 2.18.0 milestone Feb 19, 2017

assert.ok(isNaN(m.valueOf()));
});
test('valid duration - as per @ichernev', function (assert) {

This comment has been minimized.

@ichernev

ichernev Feb 19, 2017

Contributor

Well, no need for that mention, I'm famous enough already ;-)

@ichernev

This comment has been minimized.

Contributor

ichernev commented Feb 19, 2017

@marwahaha thank you!

Should I add a helper method moment.duration.invalid()? I couldn't find where moment.invalid() was implemented.

Follow the chain from src/moment.js to src/lib/moment/moment.js to src/lib/create/valid.js.

this.isValid() does not re-compute the value of this._isValid. If it's not ok, I can change it.

Well, the _isValid is an optimization. Also you should make sure that an invalid duration stays invalid (you can not perform an op and make it magically valid) -- you have tests for that. So its fine.

@ichernev ichernev referenced this pull request Feb 19, 2017

Open

v3 changes #3775

0 of 10 tasks complete
@marwahaha

This comment has been minimized.

Member

marwahaha commented Feb 21, 2017

Ok @ichernev I think I am done making revisions. Do let me know.

@ichernev ichernev removed the Needs Revision label Mar 2, 2017

@ichernev

This comment has been minimized.

Contributor

ichernev commented Mar 2, 2017

@marwahaha thank you, looks great.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Mar 2, 2017

Merged in a05fbb2

@ichernev ichernev changed the title from Create _isValid property for durations to [feature] Durations gain validity Mar 2, 2017

@ichernev ichernev closed this Mar 2, 2017

ichernev added a commit that referenced this pull request Mar 2, 2017

Merge pull request #3611 from marwahaha:patch-3225
[feature] Durations gain validity
@ichernev

This comment has been minimized.

Contributor

ichernev commented May 22, 2017

@marwahaha lets PR the v3 change agains the immutability branch.

mjomble added a commit to mjomble/flow-typed that referenced this pull request Aug 14, 2017

marudor added a commit to flow-typed/flow-typed that referenced this pull request Aug 16, 2017

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