-
Notifications
You must be signed in to change notification settings - Fork 82
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
add --verbose
flag
#415
base: master
Are you sure you want to change the base?
add --verbose
flag
#415
Conversation
The failing test (failing to parse error message on windows) seems to be unrelated to this PR. It is the same issue which is blocking the dependabot. |
I think I also fixed the path issue on windows! |
Do you mind applying that test path fix to the pending Dependabot PR? It should pass after that and I'll accept the PR. |
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.
Just a tiny change request and also one thing to think about: Is "verbose" the right name for this flag? It's not wrong but it's also pretty specific about reporting "progress" - so is something like that maybe better?
Appreciate the tests, BTW! |
Looks good - tho I can't merge it because it's a Draft PR? |
Thanks for your quick feedback and helps. I'm trying an idea (and simultaneously learning a bit of JS). I'll finalize this in a couple of days. |
I'm not sure there's a lot of room to improve upon what you have here, but I will keep an open mind until I see what you're up to. :) |
file names given to and received the library are logged independently linters’s success messages are logged to stdout and fail to stderr
@DavidAnson Thanks for keeping an open mind. 😄I hope you are not allergic to bad JS codes! Now the
As you already know, I’m not familiar with idiomatic JS. I’ll appreciate if you can guide me to improve this PR. |
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.
OK, I see what you're going for and it makes sense in the context of what I've seen from other tools! However, the new behavior raises some more questions and I've left new comments. I will need to look at this again on a bigger screen than my phone, but I think this captures all the significant feedback. Thank you!
.option('--enable [rules...]', 'Enable certain rules, e.g. --enable MD013 MD041 --') | ||
.option('--disable [rules...]', 'Disable certain rules, e.g. --disable MD013 MD041 --'); | ||
|
||
program.parse(process.argv); | ||
|
||
if (options.quiet && options.verbose) { | ||
options.verbose = false; |
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.
This makes me feel like it's one setting with three values?
@@ -256,6 +291,9 @@ const files = prepareFileList(program.args, ['md', 'markdown']).filter(value => | |||
const ignores = prepareFileList(options.ignore, ['md', 'markdown'], files); | |||
const customRules = loadCustomRules(options.rules); | |||
const diff = files.filter(file => !ignores.some(ignore => ignore.absolute === file.absolute)).map(paths => paths.original); | |||
if (options.verbose && !options.stdin) { | |||
console.log('files to check:', diff.join(' ')); |
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.
This will be all on the same line? That is harder to scan visually.
@@ -299,6 +337,10 @@ function lintAndPrint(stdin, files) { | |||
const fixResult = markdownlint.sync(fixOptions); | |||
const fixes = fixResult[file].filter(error => error.fixInfo); | |||
if (fixes.length > 0) { | |||
if (options.verbose && !options.output) { | |||
console.log('fixing', file); |
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 there is a colon above, let's use one here also.
}) | ||
); | ||
.filter(Boolean); | ||
|
||
let lintResultString = ''; |
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.
Instead of keeping this, I think it can be removed and recreated if it is needed. It looks like it is empty exactly when the strings array has no elements?
// @see {@link https://nodejs.org/dist/latest-v8.x/docs/api/process.html#process_process_exit_code} | ||
// @see {@link https://github.com/igorshubovych/markdownlint-cli/pull/29#issuecomment-343535291} | ||
process.exitCode = exitCodes.lintFindings; | ||
return `${result.file}: ✔`; |
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.
I would expect failing files to end with a little 'x'?
t.is(result.exitCode, 0); | ||
}); | ||
|
||
test('--verbose flag with --fix for incorrect file', async t => { |
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.
Missing test for verbose with broken file (no fix).
t.is(result.exitCode, 0); | ||
}); | ||
|
||
test('--output and --verbose with valid input logs nothing to console', async t => { |
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.
I could argue that verbose should always mean "verbose to console". If so, these two test cases would not be different. Just like I don't like how quiet
and verbose
overlap, disabling verbose
for output
seems not obvious?
--verbose
flag can be used to ensure linter is actually parsing the files when using globs or for general debugging.PS:
I couldn’t think of a simple solution to logs the relevant messages after each file’s name.