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 changes to run isEnabledFilter after checking if the ast has content and is #723

Merged

Conversation

saptar
Copy link

@saptar saptar commented May 18, 2016

This PR tries to resolve the issue with disable linter programatically as mentioned under feature request # 70.

As a part of the PR:

Added code block after conditional check to see if the ast has been created properly.
There are no associated warning or new test cases.

. Resolves #70

DCO 1.1 Signed-off-by: Saptarshi Dutta saptarshidutta31@yahoo.co.in

content. Resolves sasstools#70

DCO 1.1 Signed-off-by: Saptarshi Dutta <saptarshidutta31@yahoo.co.in>
@coveralls
Copy link

coveralls commented May 18, 2016

Coverage Status

Changes Unknown when pulling 5c13bf5 on saptar:feature/disable-linters into * on sasstools:feature/disable-linters*.

@saptar
Copy link
Author

saptar commented May 18, 2016

I wanted this feature to be completed and integrated as i intend to use this feature in a upcoming project and hence i pitched in with this PR. I have read through the comments from Avinash and my solution is in line with his suggestions as well.

@saptar saptar mentioned this pull request May 19, 2016
@@ -125,6 +125,9 @@ sassLint.lintText = function (file, options, configPath) {
}

if (ast.content && ast.content.length > 0) {

ruleToggles = getToggledRules(ast);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's still an issue in this file as far as I can see though

https://github.com/saptar/sass-lint/blob/5c13bf5300d58628decaa90283927221c0dbef06/index.js#L108

ruleToggles etc are passing a blank object as ast here, these declarations should be empty as part of this PR.

@DanPurdy
Copy link
Member

@saptar thanks for the contribution if you could update with what I left above I can merge this into the disable linters branch for you. 👍

Resolves # 70
DCO 1.1 Signed-off-by: Saptarshi Dutta saptarshidutta31@yahoo.co.in
@coveralls
Copy link

coveralls commented May 23, 2016

Coverage Status

Changes Unknown when pulling 8795bc7 on saptar:feature/disable-linters into * on sasstools:feature/disable-linters*.

@DanPurdy
Copy link
Member

Thanks

@DanPurdy DanPurdy merged commit cac9af1 into sasstools:feature/disable-linters May 23, 2016
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