-
Notifications
You must be signed in to change notification settings - Fork 22
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 notices to each rule doc indicating which configuration enables the rule #107
Conversation
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.
Thanks for this, looks good!
My only request is, could the test assert messages refer to the specific message we are checking, so the test output is more useful?
One option might be for expectedNotices
and unexpectedNotices
to be an array of objects instead of strings, maybe something like { noticeType, notice }
, and then the assert message would contain the notice type. (Notice type could be something like "recommended notice" or even just "recommended"/"fixable"/"two")
tests/index.js
Outdated
// Ensure that expected notices are present in the correct order. | ||
let currentLineNumber = 1; | ||
expectedNotices.forEach(expectedNotice => { | ||
assert.equal(lines[currentLineNumber], "", "has blank line before notice"); |
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 this assert message refer to the type of notice? (See overall review comment)
tests/index.js
Outdated
let currentLineNumber = 1; | ||
expectedNotices.forEach(expectedNotice => { | ||
assert.equal(lines[currentLineNumber], "", "has blank line before notice"); | ||
assert.equal(lines[currentLineNumber + 1], expectedNotice, "includes notice"); |
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 this assert message refer to the type of notice? (See overall review comment)
tests/index.js
Outdated
!fileContents.includes(FIXABLE_MSG), | ||
"does not include fixable notice" | ||
!fileContents.includes(unexpectedNotice), | ||
"does not include unexpected notice" |
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 this assert message refer to the type of notice? (See overall review comment)
82ef242
to
5b639dc
Compare
@platinumazure good idea, fixed! |
ESLint rule docs provide this same exact notice. Example: https://eslint.org/docs/rules/no-extra-semi
Also adds a test to ensure rule docs correctly include the notice.
This is a follow-up to #106 where I added a fixable notice to rule docs.