-
Notifications
You must be signed in to change notification settings - Fork 81
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 --enable + --disable CLI flags #234
Conversation
Awesome, thank you!! Some high-level thoughts:
|
@DavidAnson Thanks for the quick review. Great comments! I think I addressed all of them. Two of the tests for |
…s, let disable override enable
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.
Looking good!
README.md
Outdated
-j, --json write issues in json format | ||
-o, --output [outputFile] write issues to file (no console) | ||
-p, --ignore-path [file] path to file with ignore pattern(s) | ||
-q, --quiet do not write issues to STDOUT | ||
-r, --rules [file|directory|glob|package] custom rule files | ||
-r, --rules [file|directory|glob|package] custom rule files (default: []) |
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.
Could you add the word "include" here to be more clear now that we have enable/disable? "include custom rule files"
test/test.js
Outdated
|
||
test('--disable flag overrides --enable', async t => { | ||
const result = await execa('../markdownlint.js', | ||
['--enable', 'MD002', '--disable', 'MD002', '--config', 'default-false-config.yml', 'incorrect.md'], |
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.
To be really careful, can you swap the order of enable/disable here? Making enable be last is even more likely to leave the rule enabled.
test/test.js
Outdated
await execa('../markdownlint.js', | ||
['--disable', 'MD014', 'incorrect.md'], | ||
{stripFinalNewline: false}); | ||
// t.fail(); |
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 should not be commented.
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.
Good catch!
test/test.js
Outdated
|
||
try { | ||
await execa('../markdownlint.js', | ||
['--disable', 'MD014', 'incorrect.md'], |
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 think the test failure is due to incorrect.md being treated as part of the disable list. See https://github.com/tj/commander.js/blob/master/docs/options-taking-varying-arguments.md#alternative-put-options-last for one way to avoid this, tho "--" is also fine and maybe good to show here.
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.
Yeah, I tried adding the --
in there before but it didn't help. Or rather it wasn't enough. Turns out, you also need to pass each rules as a separate string for them to parse into an array, i.e.
- ['--disable', 'MD014 MD014 MD022', '--', 'incorrect.md']
+ ['--disable', 'MD014', 'MD014', 'MD022', '--', 'incorrect.md']
Do you prefer separating rules with spaces or commas? Spaces avoids the need for a custom parsing function but has slightly worse usability imo.
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.
Sweet!
By the way, I just now saw notifications for your most recent comments and updates. Sorry about seeming to ignore them, that wasn't the intent! |
@DavidAnson No problem. Do you know when the next release might land? |
Soon. I did some preparation work last night. :) |
Closes #214.