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

Feature Request: Enable Disabling Linters via Source #70

Closed
mherchel opened this issue Sep 1, 2015 · 42 comments · Fixed by #677
Closed

Feature Request: Enable Disabling Linters via Source #70

mherchel opened this issue Sep 1, 2015 · 42 comments · Fixed by #677

Comments

@mherchel
Copy link

mherchel commented Sep 1, 2015

SCSS-lint has the ability to disable the linters within the source files

https://github.com/brigade/scss-lint#disabling-linters-via-source

This is unfortunately important for my project's workflow :)

@Snugug
Copy link
Member

Snugug commented Sep 1, 2015

I'm sorry to hear that 😝

This isn't a high priority at the moment with a slew of rules that need to be written and some bugs to be squashed, but I'd be happy to take a PR for it. I'd expect the abstraction for determining if a rule should be enabled/disabled to live in helpers

@cgtm
Copy link

cgtm commented Oct 6, 2015

Yeah, this would be hugely useful for the large project I'm working on. We've only recently started linting our scss files (I know!) and we've been using scss-lint, peppering all of our current files with overrides to get them to pass for now (as described by mherchel above).

As we're moving from scss-lint to sass-lint (ditching as much ruby stuff as possible), it would be great to have this functionality. Otherwise I can see needing to create multiple rulesets for different sets of files (being loose with everything already in production and tight with all new stuff).

I think there's going to be a bit move away from all the ruby based stuff (ruby-sass, compass, scss-lint, etc.), so this will be more and more of a 'requirement' for folks to ease the transition.

So, +1 from me!

@benthemonkey
Copy link
Member

This is definitely something that should be saved for when we majorly restructure the code to perform a single traversal of the AST (see #5)

@ctumolosus
Copy link

Also, useful if you want people to use this package while gonzales-pe is trying to figure out how to solve #321 without having to ignore the whole file. I have the same issue using other tools that rely on it, but they provide a way to disable/enable sections of the source code. I've been waiting for interpolation to be fixed in gonzales-pe for a while.

@DanPurdy
Copy link
Member

I'm afraid this wouldn't solve that problem @ctumolosus. The issue is is that even with this feature gonzales-pe would still throw an error on interpolated code even if it was set to be ignored. We can't ignore code without first parsing it to know what it is. gonzales-pe parses the whole file and then we act upon what it returns. The way you are looking at this working to ignore interpolation would actually need to be a gonzales-pe feature to ignore code as it parses it.

@natorojr
Copy link

FWIW, I'm another sass-lint user in desperate need of this feature.

Glad to see it's being worked on / considered.

@mknadler
Copy link

👍, currently needing to manually comment out lines with !important flags in order to run linter, which is a blocker for integrating it fully into pre-commit hooks + build scripts

ETA: problem was actually with a nested comment

@digitaldonkey
Copy link

I'm waiting this feature desperately too. I can help testing :)

@gitnik
Copy link

gitnik commented Nov 11, 2015

Big +1 for this. Also willing to help with the implementation if steered into the right direction

@donabrams
Copy link
Contributor

I'm working on this today, should have a pull by 5pm MST

@DanPurdy
Copy link
Member

Ha, i've been working on this too, had a few bits working but the amount of tests needed here will be quite extensive and also some of the line numbers seem to cause issues also.

@bgriffith
Copy link
Member

Get talking guys! We don't want any wasted effort 😛

@DanPurdy
Copy link
Member

So the way i was approaching it was to have a rule setup similar to the comments rule, it would make a note of which context to ignore and keep a copy of the line number and then before the results are output it would filter through the results object and remove any of the results that matched those from the ignore rule.

It was also taking into account enabling and disabling rules between two blocks such as with scss-lint which worked the same way essentially as above but the different contexts were tough to get exactly right.

@DanPurdy
Copy link
Member

Happy for you to carry on with this if you can though as I've not had the time recently.

@DanPurdy
Copy link
Member

I like the extra rule there to enforce the use of documented comments with them. As for the implementation this exactly matches the way scss-lint handles it right?

rschamp pushed a commit to rschamp/scratch-www that referenced this issue Mar 23, 2016
These warnings were annoying me because the noise makes it easy to miss real issues.

The `no-mergeable-selectors` rule is one that we do want to have, but right now it asks that you merge selectors in different `@media` blocks.  When the next release happens we should put that back.

sasstools/sass-lint#307

Similarly, we want `force-element-nesting` but there is a problem with that because there's no easy way to have a nested selector in a list of selectors.
sasstools/sass-lint#575

Finally, until they implement per-line overrides, we have to silence `class-name-format` because we don't have control over the ReactModal class names.  It's a useful rule to keep class names consistent though.  Per-line ignores should be coming soon: sasstools/sass-lint#70
@donabrams
Copy link
Contributor

Hey, so finally have time to finish this feature. What needs to be done to close this up?

@DanPurdy
Copy link
Member

DanPurdy commented May 9, 2016

Hey @donabrams There's been some big AST changes so it looks like we just need to go through and check everything again, Sass format should now be a bit more stable with its interpretations of blocks so once that format is working correctly we can review what's been done. I intended to get a bit of extra work done on this recently but as usual time is swallowed up elsewhere.

I merged all of your existing progress into our disable linters branch and opened up #677 into develop to keep the progress in check. So all new PR's should be opened into the disable-linters branch and this will update accordingly.

saptar pushed a commit to saptar/sass-lint that referenced this issue May 18, 2016
content. Resolves sasstools#70

DCO 1.1 Signed-off-by: Saptarshi Dutta <saptarshidutta31@yahoo.co.in>
saptar pushed a commit to saptar/sass-lint that referenced this issue May 18, 2016
content. Resolves sasstools#70

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

albell commented May 26, 2016

+1. Excited to switch over to this linter in the near future but this issue is blocking for our team. Thanks for all the hard work.

@peduarte
Copy link

Hey guys,
I'm running sass-lint@1.8.2 and linter toggle via comment doesn't seem to be working. I've gone through the issues and pull requests and I cannot tell whether it's been implemented or still WIP?

Could someone please let me know?

Thanks! 👍

@DanPurdy
Copy link
Member

It's not been released yet, still WIP in #677 hoping to find some time soon to look into it...

@peduarte
Copy link

@DanPurdy ok thanks

@todd-richmond
Copy link

See [http://stackoverflow.com/questions/35136098/have-sass-lint-ignore-a-certain-line] for a thread on this as well

For me, I need to import Google Material Design icons, but the documented css logic causes no-duplicate-properties and no-vendor-prefixes to fire. I therefore want to keep those rules for every other file in my tree except my font.scss file

// http://google.github.io/material-design-icons/
@font-face {
  font-family: 'Material Icons';
  font-style: normal;
  font-weight: 400;
  src: url('~material-design-icons/iconfont/MaterialIcons-Regular.eot'); // IE
  src: local('Material Icons'),
  local('MaterialIcons-Regular'),
  url('~material-design-icons/iconfont/MaterialIcons-Regular.woff2') format('woff2'),
  url('~material-design-icons/iconfont/MaterialIcons-Regular.woff') format('woff'),
  url('~material-design-icons/iconfont/MaterialIcons-Regular.ttf') format('truetype');
}

.material-icons {
  // Firefox
  -moz-osx-font-smoothing: grayscale;
  // WebKit
  -webkit-font-smoothing: antialiased;
  direction: ltr;
   ...
}

@smnbbrv
Copy link

smnbbrv commented Aug 6, 2016

Hi guys, this issue seems to have a long story behind and it does not look like it will be fixed in the closest future.

Is there a way to use a beta / alpha / whatever version, might be not even synced with master one? I would be totally fine with that.

DanPurdy pushed a commit to DanPurdy/sass-lint that referenced this issue Aug 8, 2016
content. Resolves sasstools#70

DCO 1.1 Signed-off-by: Saptarshi Dutta <saptarshidutta31@yahoo.co.in>
DanPurdy pushed a commit that referenced this issue Aug 23, 2016
content. Resolves #70

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

+1 for this feature!

@DanPurdy
Copy link
Member

@jackblackCH please see #677 should be available in the next release

@smnbbrv if you want to point your package file at the branch in #677 you can use this functionality right now and we'd appreciate the testing feedback also.

DanPurdy pushed a commit that referenced this issue Oct 28, 2016
content. Resolves #70

DCO 1.1 Signed-off-by: Saptarshi Dutta <saptarshidutta31@yahoo.co.in>
DanPurdy pushed a commit that referenced this issue Nov 2, 2016
content. Resolves #70

DCO 1.1 Signed-off-by: Saptarshi Dutta <saptarshidutta31@yahoo.co.in>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.