Add SCSS configuration for Hound #17769

Merged
merged 1 commit into from Oct 4, 2015

3 participants

@croaky
croaky commented Oct 4, 2015

What

This change uses Bootstrap's existing scss/.scss-lint.yml file to configure Hound's hosted SCSS-Lint instance.

It leaves existing JavaScript linting as-is because Hound does not support ESLint or JSCS yet. It does support JSHint for the master branch, but I decided to focus just on v4 for this change.

How it works

On each pull request to Bootstrap, Hound will comment on any SCSS style violations in-line, like this:

screenshot

If you update the pull request to adopt a suggestion, the comment will be hidden.

The same checks are maintained on pull requests. Instead of output such as this in Travis...

scss/_navbar.scss:216 [W] TrailingWhitespace: Line contains trailing whitespace

... it will be an in-line comment from Hound.

Hound is free for open source projects and is open source itself, like Travis: https://github.com/thoughtbot/hound

Why

The advantages for the Bootstrap project might be:

  • Ruby does not need to be installed on developer's machines in order for grunt test to run all lint checks.
  • If jekyll:docs could be removed from the build, Ruby would not need to be installed on TravisCI.
  • Hound runs in parallel to Travis, speeding up the time CI finishes by about 4.6s on builds such as https://travis-ci.org/twbs/bootstrap/jobs/83432163
  • For parity with your current setup, I've set the fail_on_violations: true setting, but if SCSS-Lint has false positives or false negatives, you can change the setting to false to treat its comments more as suggestions than a binary outcome that should fail the build. https://houndci.com/configuration#configuration
@cvrebert
Bootstrap member
cvrebert commented Oct 4, 2015
@mdo
Bootstrap member
mdo commented Oct 4, 2015

I'm in favor. Been using it on Primer and haven't had much issue with it.

@cvrebert
Bootstrap member
cvrebert commented Oct 4, 2015

@croaky Am I correct in assuming that Hound only lints changed files? My point being that if something failing lint gets pushed directly to master, we'd never find out about it via Hound? If yes, then I'm opposed to removing the scsslint task from the Gruntfile.

But in either case, I'm +1 on enabling Hound.

@croaky
croaky commented Oct 4, 2015

Am I correct in assuming that Hound only lints changed files?

Yup, that's correct. If you're interested, the security doc goes into excruciating detail of the what happens but the gist is it runs on the full file contents of non-deleted files in the pull request.

My point being that if something failing lint gets pushed directly to master, we'd never find out about it via Hound?

Yup, also correct. Hound only operates on GitHub webhook events for opening a pull request, and pushing follow-on commits to the pull request.

If yes, then I'm opposed to removing the scsslint task from the Gruntfile.

Sounds good. Want me to revert that deletion?

@cvrebert
Bootstrap member
cvrebert commented Oct 4, 2015

Sounds good. Want me to revert that deletion?

Yes please.

@croaky croaky Add SCSS configuration for Hound
This change uses Bootstrap's existing `scss/.scss-lint.yml` file
to configure Hound's hosted SCSS-Lint instance.

On each pull request to Bootstrap,
Hound will comment on any SCSS style violations in-line, like this:

![screenshot](https://images.thoughtbot.com/hound/scss-example.png)

If you update the pull request to adopt a suggestion,
the comment will be hidden.

It leaves the existing linting done by Grunt + Travis.

Hound is free for open source projects
and is open source itself:

https://github.com/thoughtbot/hound
a7b37dc
@croaky croaky changed the title from Add configuration for Hound to Add SCSS configuration for Hound Oct 4, 2015
@croaky
croaky commented Oct 4, 2015

Cool, updated.

Since the existing Grunt + Travis lines are now back, I removed the fail_on_violations: true configuration line. By default, Hound will not mark the GitHub status on the PR as failed if it finds violations, it will only comment. That seemed to make sense to me since Travis should already fail in those scenarios. But, let me know if you like it to fail on violations.

Since Hound can only be enabled for the whole repo, I think we'll want a separate set of config branched off master:

https://github.com/twbs/bootstrap/compare/twbs:master...croaky:hound-master?expand=1

I can open that PR if that plan works for you. To avoid JSHint / SCSS-Lint surprises, both PRs (one branched off master, one off v4-dev) should be merged before an admin on this repo enables Hound at https://houndci.com.

@cvrebert
Bootstrap member
cvrebert commented Oct 4, 2015

SGTM. Open it.

@cvrebert cvrebert added this to the v4.0.0-alpha.2 milestone Oct 4, 2015
@cvrebert
Bootstrap member
cvrebert commented Oct 4, 2015

Merged to master in d53525c

@cvrebert cvrebert merged commit d53943d into twbs:v4-dev Oct 4, 2015

1 check passed

Details continuous-integration/travis-ci/pr The Travis CI build passed
@cvrebert
Bootstrap member
cvrebert commented Oct 4, 2015

...And now activated on houndci.com
Thanks!

@mdo mdo referenced this pull request Oct 13, 2015
Closed

v3.3.6 ship list #16644

@cvrebert cvrebert added a commit that referenced this pull request Oct 20, 2015
@cvrebert cvrebert Fix Hound's JS config
Per #17997 (comment) ; thanks @vsn4ik !
Refs #17769
/fyi @croaky

[ci skip]
18ad53f
@cvrebert cvrebert added a commit that referenced this pull request Oct 20, 2015
@cvrebert cvrebert Fix Hound's JS config
Per #17997 (comment) ; thanks @vsn4ik !
Refs #17769
/fyi @croaky

[ci skip]
0ccac4a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment