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

3/3: Verbose Output #50

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@ var ruleFinder = getRuleFinder('path/to/eslint-config')

ruleFinder.getCurrentRules()

ruleFinder.getCurrentRulesDetailed()
Copy link
Owner

Choose a reason for hiding this comment

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

Strange!! why is this still in diff?

Do you need to rebase this with upstream/master?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, probably just forgot to rebase m(


ruleFinder.getPluginRules()

ruleFinder.getAllAvailableRules()
Expand Down
121 changes: 100 additions & 21 deletions src/bin/diff.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,41 +2,120 @@

'use strict'

var path = require('path')
var argv = require('yargs')
.boolean('verbose')
.alias('v', 'verbose')
.argv

var size = require('window-size')
var availableWidth = size.width || /* istanbul ignore next */ 80
var ui = require('cliui')({width: availableWidth})

var getRuleFinder = require('../lib/rule-finder')
var difference = require('../lib/array-diff')
var arrayDifference = require('../lib/array-diff')
var objectDifference = require('../lib/object-diff')
var getSortedRules = require('../lib/sort-rules')

var rules = getSortedRules(
difference(
getRuleFinder(process.argv[2]).getCurrentRules(),
getRuleFinder(process.argv[3]).getCurrentRules()
)
)

var outputRules, outputRuleCellMapper
var outputPadding = ' '
var outputMaxWidth = 0
var outputMaxCols = 0

var files = [argv._[0], argv._[1]]
var collectedRules = getFilesToCompare(files).map(compareConfigs)

var rulesCount = collectedRules.reduce(
function getLength(prev, curr) {
return prev + (curr && curr.rules ? curr.rules.length : /* istanbul ignore next */ 0)
}, 0)

var outputRuleCellMapper

/* istanbul ignore next */
if (rules.length) {
console.log('\n diff rules\n') // eslint-disable-line no-console
rules = rules.map(function columnSpecification(rule) {
rule = rule + outputPadding
outputMaxWidth = Math.max(rule.length, outputMaxWidth)
return rule
})
outputMaxCols = Math.floor(availableWidth / outputMaxWidth)
if (rulesCount) {
console.log('\ndiff rules') // eslint-disable-line no-console

outputMaxCols = argv.verbose ? 3 : Math.floor(availableWidth / outputMaxWidth)
outputRuleCellMapper = getOutputRuleCellMapper(Math.floor(availableWidth / outputMaxCols))
while (rules.length) {
outputRules = rules.splice(0, outputMaxCols).map(outputRuleCellMapper)
ui.div.apply(ui, outputRules)

collectedRules.forEach(function displayConfigs(config) {
var rules = config.rules

if (!rules.length) {
return
}

if (argv.verbose) {
rules.unshift('', config.base, config.head)
} else {
console.log( // eslint-disable-line no-console
'\nin ' + config.base + ' but not in ' + config.head + ':\n' +
rules.length + ' rules found\n'
)
}

while (rules.length) {
ui.div.apply(
ui,
rules.splice(0, outputMaxCols).map(outputRuleCellMapper)
)
}
console.log(ui.toString()) // eslint-disable-line no-console
})
}

function getFilesToCompare(allFiles) {
var filesToCompare = [allFiles]
Copy link
Owner

Choose a reason for hiding this comment

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

this is an array inception, allFiles is already and array.

var files = [argv._[0], argv._[1]]
var collectedRules = getFilesToCompare(files).map(compareConfigs)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But it's currently required.

  • files specifies the files to compare
  • filesToCompare describes which files to compare, and in what order

For non-verbose output, we need to do two comparisons in total, first compare a to b, second compare b to a.


if (!argv.verbose) {
// in non-verbose output mode, compare a to be
// and b to a afterwards, to obtain ALL differences
// across those files, but grouped
filesToCompare.push([].concat(allFiles).reverse())
}

return filesToCompare
}

function compareConfigs(currentFiles) {
var rules = rulesDifference(
getRuleFinder(currentFiles[0]),
getRuleFinder(currentFiles[1])
)

return {
base: path.basename(currentFiles[0]),
head: path.basename(currentFiles[1]),
rules: rules.map(transformToColumnSpecification),
}
}

function rulesDifference(a, b) {
if (argv.verbose) {
return getSortedRules(
objectDifference(
a.getCurrentRulesDetailed(),
b.getCurrentRulesDetailed()
)
)
}

return getSortedRules(
arrayDifference(
a.getCurrentRules(),
b.getCurrentRules()
)
)
}

function transformToColumnSpecification(rule) {
/* istanbul ignore if */
if (typeof rule !== 'string') {
rule = JSON.stringify(rule)
}
console.log(ui.toString()) // eslint-disable-line no-console
rule = rule + outputPadding
outputMaxWidth = Math.max(rule.length, outputMaxWidth)
return rule
}

function getOutputRuleCellMapper(width) {
Expand Down
6 changes: 5 additions & 1 deletion src/bin/find.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ var options = {
getPluginRules: ['plugin', 'p'],
getAllAvailableRules: ['all-available', 'a'],
getUnusedRules: ['unused', 'u'],
verbose: ['verbose', 'v'],
n: ['no-error'],
}

Expand All @@ -32,9 +33,12 @@ Object.keys(options).forEach(function findRules(option) {
var ruleFinderMethod = ruleFinder[option]
if (argv[option] && ruleFinderMethod) {
rules = ruleFinderMethod()
argv.verbose && console.log( // eslint-disable-line no-console
Copy link
Owner

Choose a reason for hiding this comment

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

Something to be considered as a follow up.
cyclomatic complexity of findRules function has increased over the time, it should be refactored.
Not in this PR

'\n' + options[option][0] + ' rules\n' + rules.length + ' rules found\n'
)
/* istanbul ignore next */
if (rules.length) {
console.log('\n' + options[option][0], 'rules\n') // eslint-disable-line no-console
!argv.verbose && console.log('\n' + options[option][0] + ' rules\n') // eslint-disable-line no-console
rules = rules.map(function columnSpecification(rule) {
rule = rule + outputPadding
outputMaxWidth = Math.max(rule.length, outputMaxWidth)
Expand Down
27 changes: 27 additions & 0 deletions src/lib/object-diff.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
var assert = require('assert')

function difference(a, b) {
var diff = {}

Object.keys(a).forEach(compare(diff, a, b))
Object.keys(b).forEach(compare(diff, a, b))

return diff
}

function compare(diff, a, b) {
return function curried(n) {
if (!diff[n]) {
try {
assert.deepEqual(a[n], b[n])
} catch (e) {
diff[n] = {
config1: a[n],
config2: b[n],
Copy link
Owner

Choose a reason for hiding this comment

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

Should we prune the a[n] and b[n] from a and b respectively, after this step? Or at least b[n] from b, otherwise we will be stepping over the config which has already been considered again for

Object.keys(b).forEach(compare(diff, a, b))

Copy link
Owner

Choose a reason for hiding this comment

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

If we do that, I guess we don't need the if (!diff[n]) any more.

Copy link
Owner

Choose a reason for hiding this comment

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

It's ok to address this in a subsequent PR, provided that there is an issue opened for this. 😈

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's an interesting point, I'll consider when pulling object-diff from this PR.

}
}
}
}
}

module.exports = difference
5 changes: 5 additions & 0 deletions src/lib/rule-finder.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,11 @@ function RuleFinder(specifiedFile) {
return getSortedRules(currentRules)
}

// get all the current rules' particular configuration
this.getCurrentRulesDetailed = function getCurrentRulesDetailed() {
return config.rules
}

Copy link
Owner

Choose a reason for hiding this comment

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

Strange!! why is this still in diff?

Do you need to rebase this with upstream/master?

// get all the plugin rules instead of referring the extended files or documentation
this.getPluginRules = function getPluginRules() {
return getSortedRules(pluginRules)
Expand Down
24 changes: 21 additions & 3 deletions src/lib/sort-rules.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,27 @@
'use strict'

function getSortedRules(rules) {
return rules.sort(function sort(a, b) {
return a > b ? 1 : -1
})
return Array.isArray(rules) ? getSortedRulesArray(rules) : transformIntoSortedRulesArray(rules)
}

function getSortedRulesArray(rules) {
return rules.sort(sortAlphabetically)
}

function transformIntoSortedRulesArray(rules) {
var sortedRules = []

Object.keys(rules)
.sort(sortAlphabetically)
.forEach(function map(ruleName) {
Copy link
Owner

Choose a reason for hiding this comment

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

Please use a different name than map

sortedRules.push(ruleName, rules[ruleName].config1, rules[ruleName].config2)
Copy link
Owner

Choose a reason for hiding this comment

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

Am I reading this correct?

// sortedRules
['foo-rule', '0 foo', '2 foo', 'bar-rule', '2 bar', '1 bar', ...]

If I am reading this correct, I am afraid, it would not be difficult to maintain. I would expect it to be

[{ 'foo-rule': {config1: '0 foo', config2:  '2 foo'} }, ....]

Copy link
Owner

@sarbbottam sarbbottam Apr 18, 2016

Choose a reason for hiding this comment

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

I am confused 😕

Why does this is look different to me than

{'foo-rule': {config1: [2, 'foo'], config2: [2, 'bar']}

Copy link
Owner

Choose a reason for hiding this comment

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

I guess, now I understand

['bar-rule', '1', '2', 'baz-rule', '3', '4']

Looks like, bin/diff.js expects this data-structure, I am yet to go through the bin/diff.js

If yes, I would request to use non-positional schema.

Copy link
Owner

Choose a reason for hiding this comment

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

I guess you are preferring this schema for cliui. I would have moved the cliui related code to an util file, and delegate the transformation job to it (for both find and diff, verbose and non verbose).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Making the module's output to read

[{ 'foo-rule': {config1: '0 foo', config2:  '2 foo'} }, ....]

sounds good, let that cliui util do the necessary flattening.

})

return sortedRules
}

function sortAlphabetically(a, b) {
return a > b ? 1 : -1
}

module.exports = getSortedRules
25 changes: 22 additions & 3 deletions test/bin/diff.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,18 @@ var sinon = require('sinon')

var consoleLog = console.log // eslint-disable-line no-console

var difference = sinon.stub().returns(['diff'])
var arrayDifference = sinon.stub().returns(['diff'])
var objectDifference = sinon.stub().returns(['diff'])

var stub = {
'../lib/rule-finder': function() {
return {
getCurrentRules: function noop() {},
getCurrentRulesDetailed: function noop() {},
}
},
'../lib/array-diff': difference,
'../lib/array-diff': arrayDifference,
'../lib/object-diff': objectDifference,
}

describe('diff', function() {
Expand All @@ -23,18 +26,34 @@ describe('diff', function() {

afterEach(function() {
console.log = consoleLog // eslint-disable-line no-console
// purge yargs cache
delete require.cache[require.resolve('yargs')]
})

it('log diff', function() {
process.argv[2] = './foo'
process.argv[3] = './bar'
console.log = function() { // eslint-disable-line no-console
if (arguments[0].match(/(diff|but not in|rules found)/)) {
return
}
consoleLog.apply(null, arguments)
}
proxyquire('../../src/bin/diff', stub)
assert.ok(arrayDifference.called)
})

it('verbose log diff', function() {
process.argv[2] = './foo'
process.argv[3] = './bar'
process.argv[4] = '-v'
console.log = function() { // eslint-disable-line no-console
Copy link
Owner

Choose a reason for hiding this comment

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

You can club both the console.log = function() in beforeEach

if (arguments[0].match(/(diff)/)) {
return
}
consoleLog.apply(null, arguments)
}
proxyquire('../../src/bin/diff', stub)
assert.ok(difference.called)
assert.ok(objectDifference.called)
})
})
9 changes: 8 additions & 1 deletion test/bin/find.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ var stub = {
describe('bin', function() {
beforeEach(function() {
console.log = function() { // eslint-disable-line no-console
if (arguments[0].match(/(current|plugin|all\-available|unused)/)) {
if (arguments[0].match(/(current|plugin|all\-available|unused|rules found)/)) {
return
}
consoleLog.apply(null, arguments)
Expand Down Expand Up @@ -86,4 +86,11 @@ describe('bin', function() {
proxyquire('../../src/bin/find', stub)
assert.ok(getUnusedRules.called)
})

it('verbose log', function() {
process.argv[2] = '-c'
process.argv[3] = '--verbose'
proxyquire('../../src/bin/find', stub)
assert.ok(getCurrentRules.called)
})
})
27 changes: 27 additions & 0 deletions test/lib/object-diff.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
var assert = require('assert')
var difference = require('../../src/lib/object-diff')

describe('object difference', function() {
it('should return difference', function() {
assert.deepEqual(
difference({'foo-rule': [2, 'foo']}, {'foo-rule': [2, 'bar']}),
{'foo-rule': {config1: [2, 'foo'], config2: [2, 'bar']}}
)
assert.deepEqual(
difference({'foo-rule': [2, 'foo', 'bar']}, {'foo-rule': 2}),
{'foo-rule': {config1: [2, 'foo', 'bar'], config2: 2}}
)
assert.deepEqual(
difference({'foo-rule': [0, 'foo']}, {'bar-rule': [1, 'bar']}),
{
'foo-rule': {config1: [0, 'foo'], config2: undefined},
'bar-rule': {config1: undefined, config2: [1, 'bar']},
}
)

assert.deepEqual(
difference({'foo-rule': [1, 'foo', 'bar']}, {'foo-rule': [1, 'foo', 'bar']}),
{}
)
})
})
19 changes: 19 additions & 0 deletions test/lib/rule-finder.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,15 @@ describe('rule-finder', function() {
assert.deepEqual(ruleFinder.getCurrentRules(), ['foo-rule'])
})

it('no specifiedFile - current rule config', function() {
var ruleFinder
process.cwd = function() {
return noSpecifiedFile
}
ruleFinder = getRuleFinder()
assert.deepEqual(ruleFinder.getCurrentRulesDetailed(), {'foo-rule': [2]})
})

it('no specifiedFile - plugin rules', function() {
var ruleFinder
process.cwd = function() {
Expand Down Expand Up @@ -76,6 +85,11 @@ describe('rule-finder', function() {
assert.deepEqual(ruleFinder.getCurrentRules(), ['bar-rule', 'foo-rule'])
})

it('specifiedFile (relative path) - current rule config', function() {
var ruleFinder = getRuleFinder(specifiedFileRelative)
assert.deepEqual(ruleFinder.getCurrentRulesDetailed(), {'bar-rule': [2], 'foo-rule': [2]})
})

it('specifiedFile (relative path) - plugin rules', function() {
var ruleFinder = getRuleFinder(specifiedFileRelative)
assert.deepEqual(ruleFinder.getPluginRules(), ['react/bar-rule', 'react/baz-rule', 'react/foo-rule'])
Expand All @@ -99,6 +113,11 @@ describe('rule-finder', function() {
assert.deepEqual(ruleFinder.getCurrentRules(), ['bar-rule', 'foo-rule'])
})

it('specifiedFile (absolut path) - current rule config', function() {
var ruleFinder = getRuleFinder(specifiedFileAbsolute)
assert.deepEqual(ruleFinder.getCurrentRulesDetailed(), {'foo-rule': [2], 'bar-rule': [2]})
})

it('specifiedFile (absolut path) - plugin rules', function() {
var ruleFinder = getRuleFinder(specifiedFileAbsolute)
assert.deepEqual(ruleFinder.getPluginRules(), ['react/bar-rule', 'react/baz-rule', 'react/foo-rule'])
Expand Down
Loading