-
Notifications
You must be signed in to change notification settings - Fork 34
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
Conversation
Current coverage is
|
@@ -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 |
There was a problem hiding this comment.
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
@sarbbottam what do you think about the idea of colorising the verbose diff output? In case it's a lot, printed on screen, I think it would definitely help getting the major differences at first glance ... |
🌈 👍 |
Compare the actual configurations via verbose output.
I hope this pushed changes solve most of your notes, @sarbbottam.
I'd propose to realize the colored output via its own PR, to not bring too much noise into this one (and we then could think about whether it would make sense to think about colorized output in general). |
Yep, I prefer one use-case : one PR. I will go through the PR this afternoon. |
} catch (e) { | ||
diff[n] = { | ||
config1: a[n], | ||
config2: b[n], |
There was a problem hiding this comment.
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))
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. 😈
There was a problem hiding this comment.
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.
Sorry for being so picky, but this a big change, I highly appreciate your effort for making this tool awesome 👏 . Here are few things, I would request.
Please let me know, if I am unclear or unreasonable. Thanks! |
@ta2edchimp please have a look at https://github.com/substack/text-table, might be easier that
|
Your points sound reasonable. From a quick look, even |
@sarbbottam please review #55 and #56 as soon as you got time for that and and eventually merge. |
Open issues of this PR continued in #61 and #62, @sarbbottam when you get the time, I'd appreciate you having a look on those again. Closing this now, please reopen if there's still things left to discuss 😉 |
sample output:
sample output:
I think, a nice extension for the "detailed comparison output" would be colorization:
2
and[2]
0
and[2, "always"]
undefined
in one of the configs