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

refactor: sasslint linter #2077

Merged
merged 3 commits into from Dec 6, 2018
Merged

refactor: sasslint linter #2077

merged 3 commits into from Dec 6, 2018

Conversation

sQVe
Copy link
Contributor

@sQVe sQVe commented Nov 13, 2018

Previous implementation required one to have sass-lint globally. This
allows you to have it locally, override the executable and add options.

@sQVe
Copy link
Contributor Author

sQVe commented Nov 13, 2018

I ran the tests on my system and it was OK. I'd love some pointers on how to solve the test issue CI is pointing out.

w0rp
w0rp previously requested changes Nov 15, 2018
Copy link
Member

@w0rp w0rp left a comment

Choose a reason for hiding this comment

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

Looks good to me. Add some documentation for the new options. Look at the ESLint options for good examples.

@sQVe
Copy link
Contributor Author

sQVe commented Nov 15, 2018

@w0rp Making progress. The implementation tests are now passing but failing for the added docs. I'm a bit unsure on why.

Previous implementation required one to have sass-lint globally. This
allows you to have it locally, override the executable and add options.
@sQVe
Copy link
Contributor Author

sQVe commented Nov 28, 2018

@w0rp Gave this a new shake. Think I managed to get everything fixed now. 👍

Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

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

This look good to me, and the documentation looks excellent.

One question -- there is a lot of duplicate code here, and the documentation only covers the scss version of each variable. I think I'd prefer making the sass variants into stubs over the scss versions (I don't really see why you'd want to have different options for both), or maybe just make sass into an alias of scss. @w0rp, do you have any opinion here?

@sQVe
Copy link
Contributor Author

sQVe commented Nov 30, 2018

@neersighted 👍 I've had the exact same thoughts regarding duplication. What makes it a bit tricky is that the big difference between scss and sass makes it quite likely that you would want different options for them. I know cases where you want to specifically set a config file for legacy sass files and another for scss.

If you guys can think of a better solution I'm happy to implement it. I'd need some guidance though as I'm not super comfortable with Vimscript.

@RyanSquared
Copy link
Member

I don't see a good way to merge the two and like you said it is definitely possible people could have different options. I think this is fine to merge as-is unless anyone else sees an obvious issue. I'll possibly get to merging this sometime over the weekend.

@neersighted neersighted dismissed w0rp’s stale review December 1, 2018 05:16

documentation provided

@neersighted neersighted merged commit 2760cf7 into dense-analysis:master Dec 6, 2018
@sQVe sQVe deleted the refactor/sasslint-linter branch December 6, 2018 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants