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

Remove unused condition #4497

Merged
merged 4 commits into from Jan 5, 2020
Merged

Remove unused condition #4497

merged 4 commits into from Jan 5, 2020

Conversation

hudochenkov
Copy link
Member

Which issue, if any, is this issue related to?

Closes #4328.

Is there anything in the PR that needs further explanation?

I'm not sure I understood the problem correctly ¯\_(ツ)_/¯ But look like removing incorrect condition fixes types issue, and doesn't break any test.

@vankop
Copy link
Member

vankop commented Dec 28, 2019

Sorry for my bad explanation. Your code does not check end (right now it does not work also), but I think it is not by design. Example disabledRanges:

{
   'block-no-empty': [{start: 0, end: 0}]
}

for this range command enable all should throw error (fall in this condition) by design as I understand. Your fix ignore this behaviour

@hudochenkov
Copy link
Member Author

I guess we need tests for this first. Even failing.

ntwb
ntwb previously approved these changes Dec 29, 2019
@hudochenkov
Copy link
Member Author

@vankop do you think we can figure out the problem in the coming week? I would like to release new stylelint soon. Don't want to release another major release in case disabled ranges fix is a breaking change.

@vankop
Copy link
Member

vankop commented Jan 3, 2020

Actually I don't understand reason of throwing error here... Maybe skip command is better

@hudochenkov
Copy link
Member Author

We have only one test, where this condition is true:

it('enable all without disabling any', () => {
return testDisableRanges('/* stylelint-enable */', () => {
throw new Error('should have errored');
}).catch((err) => {
expect(err.reason).toBe('No rules have been disabled');
});
});

@vankop
Copy link
Member

vankop commented Jan 3, 2020

So we need to add test case:

/* stylelint-disable-line block-no-empty */
/* stylelint-enable */

and update code to catch error in this case =)

@hudochenkov
Copy link
Member Author

I've added test. This test isn't failing and without removing extra condition. In fact, we have no tests which would use run condition _.last(ranges.end) :) So I guess it was just a bug, that this condition was there in the first place :)

return testDisableRanges(
'a {} /* stylelint-disable-line block-no-empty */\n/* stylelint-enable */',
).then((result) => {
expect(result.stylelint.disabledRanges).toEqual({
Copy link
Member

@vankop vankop Jan 5, 2020

Choose a reason for hiding this comment

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

We must expect error here since there is nothing to enable, if I understand correctly condition with No rules have been disabled error =)

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right :)

Copy link
Member

Choose a reason for hiding this comment

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

Hah, I found a bug in ranges assignment, I will do fix

@@ -81,7 +81,7 @@ it('needlessDisables complex case', () => {
ranges: [
{ start: 5, end: 5, unusedRule: 'color-named' },
{ start: 6, end: 6, unusedRule: 'block-no-empty' },
{ start: 8, unusedRule: 'block-no-empty' },
{ start: 8, end: 10, unusedRule: 'block-no-empty' },
Copy link
Member

@vankop vankop Jan 5, 2020

Choose a reason for hiding this comment

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

Copy link
Member Author

@hudochenkov hudochenkov left a comment

Choose a reason for hiding this comment

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

Great job, @vankop! Github won't let me approve, but I approve :)

@hudochenkov hudochenkov requested a review from ntwb January 5, 2020 15:44
@hudochenkov hudochenkov dismissed ntwb’s stale review January 5, 2020 15:45

Everything has changed.

@hudochenkov hudochenkov mentioned this pull request Jan 5, 2020
7 tasks
@hudochenkov hudochenkov merged commit 7d2f1a2 into master Jan 5, 2020
@hudochenkov hudochenkov deleted the disabled-ranges-fix branch January 5, 2020 23:23
@hudochenkov
Copy link
Member Author

  • Fixed: overlapping disabled ranges edge case (#4497).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect overlapping in disabled ranges
4 participants