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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Option to disable when no .htmlhintrc file is found #172

Merged
merged 6 commits into from Aug 29, 2018

Conversation

Projects
None yet
3 participants
@ericcornelissen
Copy link
Contributor

ericcornelissen commented Dec 29, 2017

As requested in #150, implements an option to disable linter-htmllint when no .htmlhintrc file is found 馃帀

Fixes #150.

ericcornelissen added some commits Dec 29, 2017

Add config watcher for no .htmlhintrc, don't lint without config
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.
@johnwebbcole
Copy link
Collaborator

johnwebbcole left a comment

Do you think you could add a unit test for the option?

@ericcornelissen

This comment has been minimized.

Copy link
Contributor Author

ericcornelissen commented Dec 30, 2017

Of course! Should've thought about that 馃槄 I will look at it later today

Add unit tests for disable option
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.
@Arcanemagus
Copy link
Member

Arcanemagus left a comment

A few minor refactor suggestions, but nothing that would block merging whenever you want @johnwebbcole 馃槈.

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');

This comment has been minimized.

@Arcanemagus

Arcanemagus Jan 4, 2018

Member

Since this path is being used in multiple places, just move it to the top and replace all 3 uses 馃槈.

This comment has been minimized.

@ericcornelissen

ericcornelissen Jan 4, 2018

Author Contributor

Agreed 馃憤

@@ -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 () => {

This comment has been minimized.

@Arcanemagus

Arcanemagus Jan 4, 2018

Member

I'd put both of these in a describe block myself.

This comment has been minimized.

@ericcornelissen

ericcornelissen Jan 4, 2018

Author Contributor

How would you envision that? With a nested describe or a new one, and what would its description be?

This comment has been minimized.

@Arcanemagus

Arcanemagus Jan 4, 2018

Member

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 () => {
    // ...
  });
});
@ericcornelissen

This comment has been minimized.

Copy link
Contributor Author

ericcornelissen commented Apr 13, 2018

Any particular reason this PR isn't merged yet? (just asking 馃檪)

@Arcanemagus

This comment has been minimized.

Copy link
Member

Arcanemagus commented Aug 28, 2018

Hmmm, looks like I was waiting on @johnwebbcole. I'll go ahead and update/merge this.

@Arcanemagus

This comment has been minimized.

Copy link
Member

Arcanemagus commented Aug 28, 2018

I'll actually merge this later tonight as a test of the (soon to be implemented) automated deployment.

@Arcanemagus Arcanemagus merged commit 0e2bb0d into AtomLinter:master Aug 29, 2018

6 checks passed

ci/circleci: beta Your tests passed on CircleCI!
Details
ci/circleci: checkout_code Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: stable Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Arcanemagus

This comment has been minimized.

Copy link
Member

Arcanemagus commented Aug 29, 2018

馃帀 This PR is included in version 1.5.0 馃帀

The release is available on GitHub release

Your semantic-release bot 馃摝馃殌

@ericcornelissen ericcornelissen deleted the ericcornelissen:disable-when-no-config branch Aug 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment