[Github] Last commit date and commit stats #1112
Conversation
# Conflicts: # index.html # server.js # try.html
| if (steps.length in defaultColors) { | ||
| colors = defaultColors[steps.length]; | ||
| } else { | ||
| throw Error(`No default colors for ${steps.length} steps.`); |
There was a problem hiding this comment.
These should be throw new Error, not throw Error. Invoking constructors without the new keyword is deprecated.
There was a problem hiding this comment.
I can't find a reference supporting this. I used to write new Error but stopped because it's extra tokens for seemingly no value. The ES5 spec specifically permits this. Is there an ES6 reference that says otherwise?
I only bring it up because I'd enforce what you're asking for if I had a good reference.
There was a problem hiding this comment.
Ah, interesting @paulmelnikow! That's still in the ES2015 spec (https://www.ecma-international.org/ecma-262/6.0/#sec-error-constructor):
When Error is called as a function rather than as a constructor, it creates and initializes a new Error object. Thus the function call Error(…) is equivalent to the object creation expression new Error(…) with the same arguments.
I guess it's okay then. I was basing my info off an internal lint rule we have at Facebook, but perhaps the info is outdated/incorrect.
lib/color-formatters.js
Outdated
| const daysElapsed = moment().diff(moment(date), 'days'); | ||
| return colorByAge(daysElapsed); | ||
| } | ||
| exports.age = age; |
There was a problem hiding this comment.
Do
module.exports = {
age,
colorScale,
};
for consistency with the other modules you've created?
lib/text-formatters.js
Outdated
| @@ -4,6 +4,9 @@ | |||
| */ | |||
| 'use strict'; | |||
|
|
|||
| var moment = require('moment'); | |||
There was a problem hiding this comment.
I'd suggest to put this require call inside the formatDate method so you only incur the overhead of loading moment if it's really needed.
There was a problem hiding this comment.
When would avoiding that overhead be helpful?
| intervalLabel = '/week'; | ||
| break; | ||
| default: | ||
| throw Error('Unhandled case'); |
|
|
||
| describe('Color formatters', function() { | ||
| it('should step appropriately', function() { | ||
| const byPercentage = colorScale([Number.EPSILON, 80, 90, 100]); |
There was a problem hiding this comment.
What's the purpose of Number.EPSILON here?
There was a problem hiding this comment.
The semantics of colorScale are that the color shifts into the new color when the value >= the provided step value. By making this EPSILON, a value of 0.5 produces yellow. If it were 1 it would be red.
There was a problem hiding this comment.
Can you just use 0, or do you want to differentiate 0 from "any possible value after 0"?
There was a problem hiding this comment.
The existing percentage function makes 0 red and anything after that yellow. I don't see the point in differentiating between 0 and 1, which both seem terrible, though I was aiming for parity.
There was a problem hiding this comment.
I added tests at 0.5 to clarify this.
# Conflicts: # lib/text-formatters.js # server.js
|
The six affected service tests are passing locally. |
This adds badges for the last commit date and the commit activity.
Reopen of #928. Closes #897.