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 support for sass-lint (a pure node.js scss linter) #1070

Merged
merged 1 commit into from Sep 12, 2016

Conversation

@DEADB17
Copy link
Contributor

commented Sep 4, 2016

  • a current limitation is that linting is performed on the file contents
    instead of the buffer and saving is required to re-lint

The functionality was not designed but put together by looking at existing code and it might be missing obvious improvements.

@CLAassistant

This comment has been minimized.

Copy link

commented Sep 4, 2016

CLA assistant check
All committers have signed the CLA.

@DEADB17

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2016

I'll try to figure out what the errors are as well as include tests and re-submit

@DEADB17 DEADB17 closed this Sep 4, 2016

@cpitclaudel

This comment has been minimized.

Copy link
Member

commented Sep 4, 2016

Hi @DEADB17,

Thanks for your contribution! It's quite OK to leave the PR open while you work on ironing out the bugs, and we'd be happy to help, too.

Regarding your comment about having to save: have you tried source or source-inplace instead of source-original?

@DEADB17

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2016

Thanks, I'm re-opening it then. I didn't want to add noise with a patch that was not ready for merging as it is missing tests and possibly basic documentation
Regarding the option I did try source (not source-inplace) but the problem seems to be that the sass-lint bin only works on files and not std-input as far as I can tell.

@DEADB17 DEADB17 reopened this Sep 5, 2016

@cpitclaudel

This comment has been minimized.

Copy link
Member

commented Sep 5, 2016

Regarding the option I did try source (not source-inplace) but the problem seems to be that the sass-lint bin only works on files and not std-input as far as I can tell.

std-input is independent from source/source-inplace; the former indicates whether the file should be fed to the linter, whereas the latter describes a command line argument; I'd be surprised if sass-lint doesn't work with source (or at worse source-inplace, but not source-original) :)

I'd suggest trying it again; let me know if you run into any issues.

@cpitclaudel

This comment has been minimized.

Copy link
Member

commented Sep 5, 2016

The failing test is the documentation, btw; it'll pass once you add a documentation bit to your patch (it doesn't need to be big, btw; just a reference to the checker) :) A reference to the checker in the change log would be welcome as well.

@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2016

@DEADB17 And please feel free to add yourself to the contributor list in doc/community/people.rst :)

DEADB17 added a commit to DEADB17/flycheck that referenced this pull request Sep 5, 2016

switch source-original to source
This fixes re linting on buffer changes as suggested by cpitclaudel in
flycheck#1070 (comment)
@DEADB17

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2016

@cpitclaudel thanks for the suggestion regarding source-original, changing it to source fixed the issue.
Let me know if there is anything else that should be improved.
Thanks.

flycheck.el Outdated
@@ -232,6 +232,7 @@ attention to case differences."
rust-cargo
rust
sass
sass-lint

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Sep 6, 2016

Contributor

Shouldn't we use sass before sass-lint? sass is a very basic checker that only catches some syntax errors; shouldn't we generally prefer sass-lint if it's available?

This comment has been minimized.

Copy link
@DEADB17

DEADB17 Sep 6, 2016

Author Contributor

Regarding priority, since sass-lint can be used for both sass and scss modes but it is not as developed as scss-lint yet, I think the priority should be:

  1. scss-lint
  2. sass-lint
  3. sass
  4. scss

If there are no objections I can make the change later today.

DEADB17 added a commit to DEADB17/flycheck that referenced this pull request Sep 6, 2016

@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2016

@DEADB17 I wonder whether we should probably renaming the syntax checkers following our convention for C and C++, given that both sass-lint and scss-lint check both SASS and SCSS.

How about sass/scss-sass-lint and sass/scss-scss-lint? Or are these names as awkward as I find them? 😉 Any better suggestions?

@cpitclaudel

This comment has been minimized.

Copy link
Member

commented Sep 9, 2016

I'm OK with renaming. Some part of me wants to call them s{c|a}ss-scss-lint and s{c|a}ss-sass-lint :) But jokes aside, what about s*ss-scss-lint and s*ss-sass-lint?

@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2016

@cpitclaudel Oh dear, I guess Emacs Lisp would even handle that name wouldn't it?

I'm not too convinced of s*ss either… the asterisk looks kind of strange, and what if someone publishes suss tomorrow 😜

@DEADB17

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2016

I think the value in making a decision will be: 1. validating existing conventions and 2. defining how to handle this issue in the future. Having a convention brings predictability for both users and developers.
An example where a convention will be relevant is the stylelint checker (#903), which can potentially be used for any css-like syntax, like css/scss/less-stylelint.

Quickly skimming through the code it looks like there is only one instance of a linter used for multiple modes: c/c++.
The existing convention would then be:
<mode> "/" <mode> ... "-" <linter>
Which I think will provably work better in future cases than the s*ss-sass-lint alternative which is clever but specific to this linter, thus harder to use as a convention.
The one improvement I can think of is to use a different separator for the modes and the linter:
<mode> "/" <mode> ... ":" <linter>, for example: sass/scss:sass-lint. Which would make it easier to parse if it ever needs to be automated... but honestly I'd wait until there is an actual use case.

My vote would be to use sass/scss-sass-lint.

@cpitclaudel

This comment has been minimized.

Copy link
Member

commented Sep 9, 2016

My vote would be to use sass/scss-sass-lint.

👍

@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2016

@DEADB17 Would you mind to implement the rename in this PR?

DEADB17 added a commit to DEADB17/flycheck that referenced this pull request Sep 9, 2016

@DEADB17

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2016

Done.

On Sep 9, 2016, at 11:45, Sebastian Wiesner notifications@github.com wrote:

@DEADB17 Would you mind to implement the rename in this PR?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@@ -935,6 +935,16 @@ to view the docstring of the syntax checker. Likewise, you may use

.. supported-language:: Sass

Flycheck checks SASS with `sass-lint`, falling back to `sass`.

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Sep 10, 2016

Contributor

sass-lint -> sass/scss-sass-lint.

@@ -987,7 +997,8 @@ to view the docstring of the syntax checker. Likewise, you may use

.. supported-language:: SCSS

Flycheck checks SCSS with `scss-lint`, falling back to `scss`.
Flycheck checks SCSS with `scss-lint`, falling back to `sass-lint` first and

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Sep 10, 2016

Contributor

sass-lint -> sass/scss-sass-lint.

flycheck.el Outdated
:package-version '(flycheck . "0.30"))

(flycheck-define-checker sass/scss-sass-lint
"A SCSS syntax checker using sass-Lint.

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Sep 10, 2016

Contributor

SASS/SCSS?

@DEADB17

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2016

All set

@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2016

@DEADB17 LGTM now.

scala
scala-scalastyle
scheme-chicken
scss-lint
sass/scss-sass-lint
sass

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel Sep 11, 2016

Member

I think we should keep these sorted alphabetically :)

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Sep 11, 2016

Contributor

Am 11.09.2016 um 15:49 schrieb Clément Pit--Claudel notifications@github.com:

In flycheck.el:

 scala
 scala-scalastyle
 scheme-chicken
 scss-lint
  • sass/scss-sass-lint
  • sass
    I think we should keep these sorted alphabetically :)

Not possible here because scss-lint must come first.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel Sep 11, 2016

Member

Oooh, true! Thanks :)

flycheck.el Outdated
@@ -8542,6 +8543,24 @@ See URL `http://sass-lang.com'."
line-end))
:modes sass-mode)

(flycheck-def-config-file-var flycheck-sass-lintrc sass/scss-sass-lint ".sass-lint.yml"
:safe #'stringp
:package-version '(flycheck . "0.30"))

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel Sep 11, 2016

Member

The version number is 30, not 0.30

@cpitclaudel

This comment has been minimized.

Copy link
Member

commented Sep 11, 2016

LGTM, thanks! With a few tiny changes suggested as inline comments. Can you squash your commits?

@DEADB17 DEADB17 force-pushed the DEADB17:sass-lint branch from a0100cf to 50bf869 Sep 11, 2016

@DEADB17

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2016

I updated the version and squashed.
Thanks for all the suggestions and maintaining this project!

@lunaryorn lunaryorn merged commit d4016e0 into flycheck:master Sep 12, 2016

3 checks passed

approvals/lgtm this commit looks good
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2016

@DEADB17 Merged. Many thanks for the patch, and for patiently enduring our nit-picky reviews 😊

@DEADB17

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2016

Any time 👍

@DEADB17 DEADB17 deleted the DEADB17:sass-lint branch Oct 13, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.