Date enhancements and support for Unix Timestamps #791

galenandrew · 2015-12-31T20:16:52Z

Revised

This PR includes the following changes:

Support for decimals in date() validation
New constraint for date.timestamp() that supports both javascript and unix timestamps
Updated Tests, API documentation, and added date example

Note: Because javascript timestamps are millisecond-based and unix timestamps are second-based, the timestamp() constraint accepts an optional type parameter to ensure proper date conversion for the given input value.

closes #789

Marsup · 2016-01-01T12:19:38Z

lib/date.js


-            value = parseInt(value, 10);
+            value = Number(value);


I changed this to the Number constructor because parseInt drops the decimals. After reviewing performance I've updated it to use parseFloat.

Marsup · 2016-01-01T13:02:01Z

Thinking about it, can't we just use the existing unit without adding any new api ?

galenandrew · 2016-01-04T22:49:22Z

@Marsup PR has been updated and downstream merged with your recent commits to master.

I've removed the approach that uses unit and simplified the new timestamp constraint implementation.

Marsup · 2016-01-05T07:18:20Z

So what happened to my last comment ?

galenandrew · 2016-01-05T13:01:30Z

I'm not sure I understand what you mean? Please explain.

Marsup · 2016-01-05T13:39:49Z

I don't think a new API is needed, just using the existing unit.

galenandrew · 2016-01-05T15:20:30Z

I agree that replicating the unit API within the date type is not needed, however I respectfully disagree about the need for a new API for timestamps. The date type has an explicit iso constraint and should have an explicit timestamp constraint as well.

This PR is clean, functional and has 100% test coverage.

galenandrew · 2016-01-05T15:24:17Z

If you have objective reasoning for a different approach on handling timestamps let's discuss it. For instance, getting rid of the type parameter in the timestamp API and using the built in unit to handle the multiplier. This would result in something like the following for unix timestamps: Joi.date().timestamp().unit('seconds').

Marsup · 2016-01-05T16:33:17Z

Joi.date().timestamp('unix') is essentially the same as Joi.date().unit('s') except you're adding a new API and a new concept of "unix timestamp" that not everyone might be familiar with. Joi.date() just defaults to unit('ms') when it's a number.

ISO is a strict format applied on a string hence its special treatment, a number is not timestamp by itself, any number is a timestamp. Actually, if you want a strict validation on that, I'm just now seeing that this could already be achieved by a Joi.date().format('X') or Joi.date().format('x').

using the built in unit to handle the multiplier

I do mean that the unit should translate to a multiplier.

FWIW I'm not questioning the quality of the PR here, just the content.

kanongil · 2016-01-05T20:50:55Z

@Marsup I am pretty sure it is a bad idea to change the parsing of a date depending on the .unit() value, given that it is only for annotation purposes.

Also, Joi.date().format('X') won't work for fractional timestamps.

Marsup · 2016-01-05T21:38:26Z

It's been for annotation so far, is there any valid reason not to use it as it already exists ?
Fractional as in x/y ? Moment seems to support floats according to their docs, I didn't test.

kanongil · 2016-01-05T22:51:55Z

Fractional, as in 123.45, which Joi never passes on to moment.

Marsup · 2016-01-05T22:53:48Z

Oh yeah, but with this patch it would.

galenandrew · 2016-01-05T23:11:32Z

While using unit to set the timestamp multiplier is a valid approach, what are the reasons we should not add an explicit timestamp constraint?

To me, Joi.date().timestamp() is more descriptive about the function of this constraint vs Joi.date().unit('s').

Marsup · 2016-01-06T18:26:31Z

I don't mind the timestamp per se, the flag behind it seems like a useless duplication of unit though. As common ground I'd propose timestamp takes whatever it can and translate it to unit, OK for everyone ?

galenandrew · 2016-01-06T19:15:36Z

That can work. So to confirm, is the following acceptable? If so, I'll update the PR accordingly.

The timestamp constraint will apply a numeric regex (/^[+-]?\d+(\.\d+)?$/) and any argument passed will set the existing unit flag (which will be used to control the multiplier).

So for instance…
Joi.date().timestamp('seconds') or Joi.date().timestamp().unit('seconds') will explicitly test for numeric values AND set the existing unit to seconds which will as a result trigger the multiplier. In other words, a stricter validation (eg non-timestamp values will fail).

Joi.date().unit('seconds') will trigger the multiplier, but not force the numeric values regex. In other words, a less strict validation (eg non-timestamp values will not fail but the multiplier may not be used based on the value format).

kanongil · 2016-01-06T20:18:37Z

I'd still argue against changing the meaning of unit(…). If you decide to go ahead, remember to flag this as a breaking change.

galenandrew · 2016-01-06T20:43:45Z

Honestly, I'm leaning that way as well. Avoiding unit feels cleaner and follows a better separation of concerns. "Piggy-backing" on unit introduces cross-over between the APIs and potential complications down the road if the functionality/purpose of one changes.

To me, the question should come down to the timestamp(...) parameter using a concept of type vs time segment – unix/javascript vs seconds/milliseconds. Type feels more descriptive, but time segment seems like it would allow for greater flexibility with the multiplier and value conversion.

Marsup · 2016-01-06T23:00:53Z

Back-channeled a bit with @kanongil, let's stick to the current form, gonna review now.

Marsup · 2016-01-06T23:03:25Z

lib/date.js

@@ -23,6 +23,12 @@ internals.isIsoDate = (() => {
        return date && (date.toString() === isoString);
    };
 })();
+internals.isTimestampAllowed = function (unit) {


Inline that, it's only used once.

galenandrew · 2016-01-06T23:43:20Z

Thanks for the review and line notes @Marsup!! I'll work on these changes and let you know when it's updated.

galenandrew · 2016-01-07T00:26:50Z

@Marsup, PR is updated. Good call on adding some additional checks for ambiguous values. I've also updated the date.base language to be timestamp agnostic.

When all looks good let me know and I'll rebase down to a single commit so it keeps the history short.

Marsup · 2016-01-07T07:31:48Z

lib/date.js

@@ -150,6 +159,19 @@ internals.Date.prototype.iso = function () {
    return obj;
 };

+internals.Date.prototype.timestamp = function (type) {
+
+    type = type ? type : 'javascript';


type = type || 'javascript'

Good call…will update.

- downstream merge and resolve conflicts - simplify timestamp API - refactor timestamp based on PR feedback - update date.base language to be timestamp generic - misc optimizations and updated tests - revert date.base error message

galenandrew · 2016-01-07T16:05:37Z

PR has been updated and rebased

Date enhancements and support for Unix Timestamps

Marsup · 2016-01-07T16:08:43Z

Thanks for the hard work.

galenandrew · 2016-01-07T16:10:19Z

You're welcome!! Thanks for the feedback and reviewing my pull request!

AdrieanKhisbe · 2016-01-07T16:28:33Z

👍

lock · 2020-01-09T12:30:46Z

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

galenandrew changed the title ~~Date/Timestamp enhancements and support for Unix Timestamps [closes #789]~~ Date/Timestamp enhancements and support for Unix Timestamps Dec 31, 2015

galenandrew mentioned this pull request Dec 31, 2015

Date() does not validate Unix timestamp with fractional seconds (decimals) #789

Closed

Marsup reviewed Jan 1, 2016
View reviewed changes

galenandrew force-pushed the timestamps branch from ccbdd54 to 78e750b Compare January 4, 2016 22:16

galenandrew changed the title ~~Date/Timestamp enhancements and support for Unix Timestamps~~ Date enhancements and support for Unix Timestamps Jan 6, 2016

Marsup added the feature New functionality or improvement label Jan 6, 2016

Marsup self-assigned this Jan 6, 2016

Marsup added this to the 7.2.0 milestone Jan 6, 2016

Marsup added the bug Bug or defect label Jan 6, 2016

Marsup reviewed Jan 6, 2016
View reviewed changes

Marsup reviewed Jan 7, 2016
View reviewed changes

galenandrew force-pushed the timestamps branch from fe16faf to fa2fe7e Compare January 7, 2016 16:04

Marsup added a commit that referenced this pull request Jan 7, 2016

Merge pull request #791 from galenandrew/timestamps

b616de6

Date enhancements and support for Unix Timestamps

Marsup merged commit b616de6 into hapijs:master Jan 7, 2016

galenandrew deleted the timestamps branch January 7, 2016 16:09

lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Date enhancements and support for Unix Timestamps #791

Date enhancements and support for Unix Timestamps #791

galenandrew commented Dec 31, 2015

Marsup Jan 1, 2016

galenandrew Jan 4, 2016

Marsup commented Jan 1, 2016

galenandrew commented Jan 4, 2016

Marsup commented Jan 5, 2016

galenandrew commented Jan 5, 2016

Marsup commented Jan 5, 2016

galenandrew commented Jan 5, 2016

galenandrew commented Jan 5, 2016

Marsup commented Jan 5, 2016

kanongil commented Jan 5, 2016

Marsup commented Jan 5, 2016

kanongil commented Jan 5, 2016

Marsup commented Jan 5, 2016

galenandrew commented Jan 5, 2016

Marsup commented Jan 6, 2016

galenandrew commented Jan 6, 2016

kanongil commented Jan 6, 2016

galenandrew commented Jan 6, 2016

Marsup commented Jan 6, 2016

Marsup Jan 6, 2016

galenandrew commented Jan 6, 2016

galenandrew commented Jan 7, 2016

Marsup Jan 7, 2016

galenandrew Jan 7, 2016

galenandrew commented Jan 7, 2016

Marsup commented Jan 7, 2016

galenandrew commented Jan 7, 2016

AdrieanKhisbe commented Jan 7, 2016

lock bot commented Jan 9, 2020

Date enhancements and support for Unix Timestamps #791

Date enhancements and support for Unix Timestamps #791

Conversation

galenandrew commented Dec 31, 2015

Revised

Marsup Jan 1, 2016

Choose a reason for hiding this comment

galenandrew Jan 4, 2016

Choose a reason for hiding this comment

Marsup commented Jan 1, 2016

galenandrew commented Jan 4, 2016

Marsup commented Jan 5, 2016

galenandrew commented Jan 5, 2016

Marsup commented Jan 5, 2016

galenandrew commented Jan 5, 2016

galenandrew commented Jan 5, 2016

Marsup commented Jan 5, 2016

kanongil commented Jan 5, 2016

Marsup commented Jan 5, 2016

kanongil commented Jan 5, 2016

Marsup commented Jan 5, 2016

galenandrew commented Jan 5, 2016

Marsup commented Jan 6, 2016

galenandrew commented Jan 6, 2016

kanongil commented Jan 6, 2016

galenandrew commented Jan 6, 2016

Marsup commented Jan 6, 2016

Marsup Jan 6, 2016

Choose a reason for hiding this comment

galenandrew commented Jan 6, 2016

galenandrew commented Jan 7, 2016

Marsup Jan 7, 2016

Choose a reason for hiding this comment

galenandrew Jan 7, 2016

Choose a reason for hiding this comment

galenandrew commented Jan 7, 2016

Marsup commented Jan 7, 2016

galenandrew commented Jan 7, 2016

AdrieanKhisbe commented Jan 7, 2016

lock bot commented Jan 9, 2020