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

Purge remainder of calls to new Date. #112

Merged
merged 1 commit into from
Jan 9, 2015
Merged

Conversation

cscott
Copy link
Contributor

@cscott cscott commented Jan 7, 2015

Use Date.now and Date.parse instead.

@@ -264,7 +264,7 @@ PCBucket.prototype.getFormatRevision = function(restbase, req) {
.then(function(apiRes) {
if (apiRes.status === 200) {
var apiRev = apiRes.body.items[0].revisions[0];
var tid = rbUtil.tidFromDate(new Date(apiRev.timestamp));
var tid = rbUtil.tidFromDate(+apiRev.timestamp);
Copy link
Contributor

Choose a reason for hiding this comment

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

+apiRev.timestamp

What does the leading plus do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Coerces to numeric, just to be safe. Below in the tidToDate method it should catch the oddball case where (say) date.getTime() is a method, but returns some non-numeric value, like {}, since isNaN(+{}) === true.

Here it will ensure that apiRev.timestamp is treated as a number, even if it happens to be a string like "123". (Note that new Date("2") does surprising things, which this code previously didn't protect against.)

@gwicke
Copy link
Member

gwicke commented Jan 7, 2015

Travis does not seem to be happy, and it doesn't look like a transient issue to me.

@cscott
Copy link
Contributor Author

cscott commented Jan 8, 2015

It's possible I screwed something up, I'll figure out how to run the test suite later tonight.

@gwicke
Copy link
Member

gwicke commented Jan 8, 2015

@cscott, to run the tests, you need to install & run cassandra locally. With that in place, it's just npm test.

@earldouglas
Copy link
Contributor

Note: there are multiple options for installing Cassandra. I found the quickest method was to download the tarball, extract it, and run bin/cassandra.

@gwicke
Copy link
Member

gwicke commented Jan 8, 2015

I'm preferring apt-get install cassandra ;)

On Wed, Jan 7, 2015 at 4:42 PM, James Earl Douglas <notifications@github.com

wrote:

Note: there are multiple options for installing Cassandra. I found the
quickest method was to download the tarball
https://cassandra.apache.org/download/, extract it, and run
bin/cassandra.


Reply to this email directly or view it on GitHub
#112 (comment).

@earldouglas
Copy link
Contributor

$ apt-cache search cassandra
pycassa-doc - Documentation for the Pycassa library
python-pycassa - Client library for Apache Cassandra

:sadface:

@gwicke
Copy link
Member

gwicke commented Jan 8, 2015

http://wiki.apache.org/cassandra/DebianPackaging

On Wed, Jan 7, 2015 at 4:45 PM, James Earl Douglas <notifications@github.com

wrote:

$ apt-cache search cassandra
pycassa-doc - Documentation for the Pycassa library
python-pycassa - Client library for Apache Cassandra

:sadface:


Reply to this email directly or view it on GitHub
#112 (comment).

Use `Date.now` and `Date.parse` instead.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.12%) when pulling e6a2eb9 on cscott:date-now-2 into 5984df8 on wikimedia:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.12%) when pulling e6a2eb9 on cscott:date-now-2 into 5984df8 on wikimedia:master.

@cscott
Copy link
Contributor Author

cscott commented Jan 8, 2015

Fixed. (apiRev.timestamp is a string; I thought it was numeric.)

gwicke added a commit that referenced this pull request Jan 9, 2015
Purge remainder of calls to `new Date`.
@gwicke gwicke merged commit 844e074 into wikimedia:master Jan 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants