-
Notifications
You must be signed in to change notification settings - Fork 19
Option to disable when no .htmlhintrc file is found #172
Option to disable when no .htmlhintrc file is found #172
Conversation
The formattig is taken from the linter-eslint (https://github.com/AtomLinter/linter-eslint) package.
Add a configuration watcher for the new disableWhenNoHtmhintConfig options (same as in linter-eslint). And prevent the linter from doing anything if no ruleset is found and the disableWhenNoHtmlhintConfig is set to `true`. Also added top level divider comments to lib/index.js, similar to src/main.js over at linter-eslint.
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.
Do you think you could add a unit test for the option?
Of course! Should've thought about that 😅 I will look at it later today |
Added two unit tests for the new 'disable when no .htmlhintrc file is found' options. One to see if a default configuration is used when no .htmlhintrc file is present and the option IS NOT set, and one to see if the file is not linted if the option IS set.
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.
A few minor refactor suggestions, but nothing that would block merging whenever you want @johnwebbcole 😉.
spec/linter-htmlhint-spec.js
Outdated
it('lints an invalid file (bad.html) with a default configuration when no config file is present', async () => { | ||
atom.config.set('linter-htmlhint.disableWhenNoHtmhintConfig', false); | ||
|
||
const bad = path.join(__dirname, 'fixtures', 'bad.html'); |
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.
Since this path is being used in multiple places, just move it to the top and replace all 3 uses 😉.
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.
Agreed 👍
spec/linter-htmlhint-spec.js
Outdated
@@ -32,4 +32,26 @@ describe('The htmlhint provider for Linter', () => { | |||
|
|||
expect(messages.length).toBe(0); | |||
}); | |||
|
|||
it('lints an invalid file (bad.html) with a default configuration when no config file is present', async () => { |
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'd put both of these in a describe
block myself.
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.
How would you envision that? With a nested describe
or a new one, and what would its description be?
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.
Nested under the one from L9 of course, and something like:
describe("The 'Disable when no HTMLHint config is found' option", () => {
it('lints files with no config when disabled', async () => {
// ..
});
it("doesn't lint files with no config when enabled", async () => {
// ...
});
});
Any particular reason this PR isn't merged yet? (just asking 🙂) |
Hmmm, looks like I was waiting on @johnwebbcole. I'll go ahead and update/merge this. |
I'll actually merge this later tonight as a test of the (soon to be implemented) automated deployment. |
🎉 This PR is included in version 1.5.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
As requested in #150, implements an option to disable linter-htmllint when no
.htmlhintrc
file is found 🎉Fixes #150.