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/toggle rules in src #402

Conversation

donabrams
Copy link
Contributor

So there are a ton more tests to add, and I am definitely having trouble finding the nearest block parent for a comment line. However, I wanted to give you an idea where I was headed.

@donabrams
Copy link
Contributor Author

This is for #70

@Snugug
Copy link
Member

Snugug commented Nov 13, 2015

Please include the certificate of origin sign off and let us know when it's ready for comment and review.

On Nov 12, 2015, at 8:15 PM, Don Abrams notifications@github.com wrote:

This is for #70


Reply to this email directly or view it on GitHub.

@donabrams
Copy link
Contributor Author

DCO 1.1 Signed-off-by: Donald Abrams donabrams@gmail.com

return getToggledRules(ast);
};

describe.only('rule toggling', function () {
Copy link
Member

Choose a reason for hiding this comment

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

Be sure to remove this .only so that Coveralls will report an accurate coverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I usually add an eslint warning for this.

@donabrams
Copy link
Contributor Author

Ready for comment and review. I'll need to make a separate issue for the doc-disabled-rules rule described in https://github.com/donabrams/sass-lint/blob/ab7e4d8ea2b8f65b09ac97a69e635f7802e758e5/docs/rules/doc-disabled-rules.md

@benthemonkey
Copy link
Member

Coveralls is unhappy that you didn't test the other branches inside of sortRange. I see that the function only exists because Gonzales-PE doesn't guarantee ordering. Is there any way you could rig the AST to be in the wrong order so we know the code handles that eventuality correctly?

Do you know anything about the performance impact of these extra traversals and checks? Visually there doesn't seem to be any significant impact. Just curious.

@DanPurdy
Copy link
Member

I'll get this into our sub branch a little later on and then work on it a little too, it's looking good though.

@benthemonkey don't worry about coveralls at the moment this isn't done and it's not going straight into develop.

@donabrams
Copy link
Contributor Author

Performance is split into two independent parts per file, detection of disable blocks and result filtering.

Detection of disable blocks is done in a single traversal. Time spent will grow linearly with # of comment blocks. No worries there.

The result filtering is completed after results are calculated. Filtering time grows at NUM_RESULTS * NUM_GLOBAL_DISABLES + (per Rule: NUM_RULE_RESULTS * NUM_RULE_DISABLES). This is theoretically worrisome, but not in reality: 1) Global disable is usually only done at the top of the page, not many times. 2) If a rule is being ignored a ton of times (say > 5), it's likely it should be ignored once at the top of the page.

@donabrams
Copy link
Contributor Author

Good call @benthemonkey

@benthemonkey
Copy link
Member

Thanks for the thorough explanation, @donabrams. Glad my suggestion caught some typos.

@DanPurdy
Copy link
Member

Ok, i'll have a look over this tomorrow, then we should get it into the feature branch we made and throw some more tests at it and maybe some more documentation. Is there anything you're still looking to add?

Once we're good with that lets get a plan together to release this.

Massive thankyou @donabrams it's looking great and is going to make sass-lint even better! Fingers crossed we may even get this out as part of 1.4.0

@benthemonkey
Copy link
Member

@donabrams This is being a bit picky, but do you mind going through and updating these commits to be authored by your GitHub account? I think it is helpful to link commits to the GitHub account of the person committing them. Plus, this way you can also show off your rank on the "Contributors" page 😄

There's a nice tool to help you do this: https://github.com/davidfokkema/git-rewrite-author

@donabrams
Copy link
Contributor Author

@benthemonkey So git-rewrite-author definitely changed way more than I hoped for. As @DanPurdy pointed out, just adding a secondary email to my account was much easier.

@benthemonkey
Copy link
Member

Sounds good. I apologize that it caused trouble. When I used that tool everything just worked. This PR looks good to me.

var first = tokens[0],
rules = tokens.slice(1);
switch (first) {
case 'sass-lint:disable':
Copy link
Member

Choose a reason for hiding this comment

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

This indentation isn't flagging up? Looks odd to me but if it's ok with eslint

@DanPurdy
Copy link
Member

I'll carry on with this tomorrow but so far it's looking excellent. For the NPE you're seeing when unable to reliably find a parent block I reckon we should just output an error on that line coming clean that we can't do what they want and suggest using line disables or something. Would that be a fair workaround for now? I'm assuming this is all going to change a little bit within the new gonzales.

Also as i found with the property count rule the .sass format is going to cause a lot of problems here as mixins and @-rules don't seem to report as being blocks and we're missing any sort of .sass tests here.

As i mentioned I'll look again tomorrow and when we're happy get this onto that feature branch and throw a lot more tests at it before we commit to putting it out.

Absolutely awesome work though @donabrams this is great!

@DanPurdy
Copy link
Member

DanPurdy commented Dec 7, 2015

Hi @donabrams sorry I never got round to this again, been a crazy couple of weeks! I'll get round to this soon and workto get the sass format tests in.

sorry again.

}
var isRuleEnabled = toggledRules.ruleEnable[ruleId]
.reduce(function (acc, toggleRange) {
return isBeforeOrSame(line, column, toggleRange[1], toggleRange[2])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should do ref counting here (ie [disable] [disable] [enable] means it's still disabled.)

@donabrams
Copy link
Contributor Author

No worries, I have things I should update on this too... Been swamped past few weeks but I'll get on this soon. Need to:

  • Fix parent block does not find to return an error
  • Move sort into helpers
  • Add ref counting for nested disables when determining enabled so we don't accidentally activate a rule (w/ test cases)

@gitnik
Copy link

gitnik commented Feb 12, 2016

Hey guys, any update on this? :)

@DanPurdy
Copy link
Member

I think once we've got the gonzales update out of the way this will be the next major priority.

@gitnik
Copy link

gitnik commented Feb 12, 2016

Alright thanks, appreciate the update

@elengart
Copy link

elengart commented Mar 8, 2016

+1 for this feature... thank you!

@floralWallpaper
Copy link

+1! Thanks!
Sorry - honestly thought it helped you guys see what was a more requested feature to address priority!

@jroman00
Copy link

+1

@DanPurdy
Copy link
Member

Please don't +1 in pull requests it doesn't make anything happen quicker.

@backflip
Copy link

And it annoys the hell out of everyone having subscribed to the issue. :)

@necramirez
Copy link

@DanPurdy
Copy link
Member

@ACOSW please please please don't use +1's read the thread before commenting we'll just delete these comments now

@danielbayerlein
Copy link

Are there any chance to merge this important issue?

@DanPurdy
Copy link
Member

@danielbayerlein unfortunately this is very out of date right now, its one of our next big hurdles to get sorted but merging this right now would probably break a lot of things. We are looking to get it moving though as we know how necessary it is. 👍

@avinashci
Copy link

avinashci commented Apr 28, 2016

I tried the merge and got just one merge conflict on my local on the index.js file. I ran the tests and i see the cli tests are failing which i believe is a setup issue on my end.

@DanPurdy
Copy link
Member

We made massive changes in 1.6 and the AST we are using had massive changes too so we need to go through and review this and throw a lot more tests at it, it will be happening soon though don't worry!

@DanPurdy DanPurdy merged commit e264461 into sasstools:feature/disable-linters May 5, 2016
@DanPurdy
Copy link
Member

DanPurdy commented May 5, 2016

Merged this into our test branch for now. Intend to work on this soon but basically updated it will alllllll of the progress we've made since this began. 1 conflict which was minimal and not really a problem. Will open a new PR into develop to see the progress here.

@DanPurdy DanPurdy mentioned this pull request May 5, 2016
@DanPurdy
Copy link
Member

DanPurdy commented May 5, 2016

Ongoing work in #677 Any work should be a PR into feature/disable-linters branch NOT develop.

Will look to update it all soon.

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.