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

Enable sass linting by default, as had been expected #6131

Merged
merged 1 commit into from Jul 20, 2017
Merged

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Jul 19, 2017

Reviewers: I don't know groovy and I am unsure how to test this. Help would be much appreciated.

The docs say that the sass linter is on by default:

* - sassLint Whether or not to run the SASS linter. Default: true

That's not the case though.

  • If options.sassLint is omitted then checking options.sassLint is null and falsy.
  • The default state was therefore no sass linting
  • Explicitly check that it has been set to false so that if the option is not provided sassLint runs

I think this may have broken here:
#5828

* If `options.sassLint` is omitted it is null and falsy.
* The default state was therefore no sass linting
* Explicitly check that it has been set to false so that if the option
is not provided sassLint runs
@fofr fofr requested a review from suzannehamilton Jul 19, 2017
@fofr
Copy link
Contributor Author

@fofr fofr commented Jul 19, 2017

@tijmenb
Copy link
Contributor

@tijmenb tijmenb commented Jul 19, 2017

This seems good to me, but maybe @suzannehamilton has other ideas on how to set defaults in the options?

Copy link
Contributor

@suzannehamilton suzannehamilton left a comment

That looks OK to me - I don't know of a better way handling it in Groovy.

And I think you're right that I broke the default true value, sorry. 😬

@fofr
Copy link
Contributor Author

@fofr fofr commented Jul 20, 2017

Because we lint all SASS, when we merge this there will be build failures on unrelated PRs, there may be some clean-up required.

@fofr fofr merged commit c1db716 into master Jul 20, 2017
2 checks passed
2 checks passed
continuous-integration/jenkins/branch This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@fofr fofr deleted the turn-on-sass-linting branch Jul 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.