feat: generated impacts as a part of rule descriptions #898
Conversation
|
|
|
@WilcoFiers - review please. Amendment as requested in place. |
build/configure.js
Outdated
| } | ||
|
|
||
| var cumulativeScores = getUniqueArr( | ||
| getScore(rule.any, true) // Any: get highest impact |
There was a problem hiding this comment.
Some squirrely comment spacing here.
There was a problem hiding this comment.
refactored so no more beautiful 
| var id = typeof check === 'string' ? check : check.id; | ||
| var definition = clone(findCheck(checks, id)); | ||
| if (!definition) { | ||
| grunt.log.error('check ' + id + ' not found'); |
There was a problem hiding this comment.
Should we stop execution and return here?
There was a problem hiding this comment.
Unsure, not a blocker so leaving as is, until I read up grunt docs.
build/configure.js
Outdated
| function parseImpactForRule(rule) { | ||
|
|
||
|
|
||
| function capitalize(s) { |
There was a problem hiding this comment.
I'm finding it difficult to review code that has nested functions like this. Github won't let me collapse this so I can't simply skip over them when reviewing the outer-most function. The point of abstracting is to let me skip over some of the details, but this way I have to consider multiple scopes when reading through this. I find this is harder to read than it even would have been if the abstractions weren't put in here at all. I'd much prefer that any function that can be moved to the top of the file be moved there.
@isner @stephendonner WDYT? Is that just me?
There was a problem hiding this comment.
While I agree, this seems to be the existing code-style. We tend to nest function declarations a lot.
Don't think this should necessarily be a blocker, but maybe others feel differently. ¯_(ツ)_/¯
There was a problem hiding this comment.
Ignoring above for now, until we reach a consensus...
build/configure.js
Outdated
| getScore(rule.any, true) // Any: get highest impact | ||
| .concat(getScore(rule.all, false)) // All: get all unique | ||
| .concat(getScore(rule.none, false)) // None: get all unique | ||
| ).sort().reverse(); //order highest to lowest |
There was a problem hiding this comment.
I would think lowest to highest. Not a big deal though.
There was a problem hiding this comment.
Sorted - lowest to highest...
build/configure.js
Outdated
| var scores = getImpactScores(checkCollection); | ||
| if(scores && scores.length) { | ||
| return onlyHighestScore | ||
| ? [Math.max.apply(null, scores)] |
There was a problem hiding this comment.
Might read better as:
[Math.max(...scores)]There was a problem hiding this comment.
This file is not es6 friendly, leaving it as is...
PR Checklist
Please check if your PR fulfills the following requirements:
Description of the changes
Does this PR introduce a breaking change?
Other information
@WilcoFiers - can you point me to the issue for the same?