Skip to content
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 SCSS configuration for Hound #17769

Merged
merged 1 commit into from
Oct 4, 2015
Merged

Add SCSS configuration for Hound #17769

merged 1 commit into from
Oct 4, 2015

Conversation

croaky
Copy link
Contributor

@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
Copy link
Collaborator

cvrebert commented Oct 4, 2015

CC: @twbs/team

@mdo
Copy link
Member

mdo commented Oct 4, 2015

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

@cvrebert
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

cvrebert commented Oct 4, 2015

Sounds good. Want me to revert that deletion?

Yes please.

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
@croaky croaky changed the title Add configuration for Hound Add SCSS configuration for Hound Oct 4, 2015
@croaky
Copy link
Contributor Author

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
Copy link
Collaborator

cvrebert commented Oct 4, 2015

SGTM. Open it.

@cvrebert cvrebert added this to the v4.0.0-alpha.2 milestone Oct 4, 2015
@cvrebert
Copy link
Collaborator

cvrebert commented Oct 4, 2015

Merged to master in d53525c

cvrebert added a commit that referenced this pull request Oct 4, 2015
Add SCSS configuration for Hound
@cvrebert cvrebert merged commit d53943d into twbs:v4-dev Oct 4, 2015
@cvrebert
Copy link
Collaborator

cvrebert commented Oct 4, 2015

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

@mdo mdo mentioned this pull request Oct 13, 2015
cvrebert added a commit that referenced this pull request Oct 20, 2015
Per #17997 (comment) ; thanks @vsn4ik !
Refs #17769
/fyi @croaky

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

[ci skip]
chiraggmodi pushed a commit to chiraggmodi/bootstrap that referenced this pull request Apr 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants