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

Fix incorrect line in max-empty-lines #3530

Merged
merged 4 commits into from Sep 9, 2018

Conversation

4 participants
@GorwinJororis
Contributor

GorwinJororis commented Aug 12, 2018

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

#2343

Is there anything in the PR that needs further explanation?

No, it's self explanatory.

@jeddy3 jeddy3 referenced this pull request Aug 13, 2018

Closed

Release 9.5.0 #3511

6 of 6 tasks complete
@jeddy3

jeddy3 approved these changes Aug 15, 2018

@GorwinJororis Many thanks for addressing this long-standing bug.

This LGTM.

@evilebottnawi You're most familiar with this rule. What do you think?

@GorwinJororis GorwinJororis referenced this pull request Aug 16, 2018

Closed

Opinions #3551

Some years ago -- never mind how log precisely -- ...
*/

This comment has been minimized.

@ntwb

ntwb Aug 19, 2018

Member

Should both forms of comments be displayed here?

i.e:

/**
 * Call me Ishmael.
 *
 *
 *
 * Some years ago -- never mind how log precisely -- ...
/*

And

/*
 Call me Ishmael.



 Some years ago -- never mind how log precisely -- ...
 */

Possibly adding some more tests to ensure both use cases are covered?

This comment has been minimized.

@GorwinJororis

GorwinJororis Aug 19, 2018

Contributor

From my point of view, this comment doesn't have empty lines (please attention to stars):

/**
 * Call me Ishmael.
 *
 *
 *
 * Some years ago -- never mind how log precisely -- ...
 */

That's why I removed from readme.md.

This comment has been minimized.

@hudochenkov

hudochenkov Aug 19, 2018

Member

I agree with @GorwinJororis. This example doesn't have empty lines.

checkMatch(source, match.endIndex, comment, 2);
});
});
}

This comment has been minimized.

@ntwb

ntwb Aug 19, 2018

Member

Is the above section supposed to have been removed?

Won't the warning position for SCSS be inaccurate with this removed?

This comment has been minimized.

@GorwinJororis

GorwinJororis Aug 19, 2018

Contributor

@ntwb Well you found something right that there will be false warning position in non-standard syntaxes but it's not related to what you pointed above. Because above is just a part which looks for comments in input css string based on ignore: ['comments'] option which I already did after few lines later.

Beside that problem is root.toString() . Because it cannot be used to check css string. Instead of you have to use PostCSS' node functions to achieve this.

Then there is one more problem. It's same problem why I couldn't in my first PR and also now in my second PR because PostCSS' node tree structure is so crap.

For example, in node tree, before is the space symbols before the node. It also stores * and _ symbols before the declaration (IE hack), after is the space symbols after the last child of the node to the end of the node, they say. WHY?

/* before goes here */
.a {
  color: #fff;
  /* after goes here */
}

Seriously WHY? Why before is outter but after is inner? Maybe it's my personal thought but this is how I think. So for final words, don't wait for me if you think you can fix this. You are 100% free to edit my PR to fix this solution. Only thing I know that this bug stands for over one year and has to be fixed.

@ntwb

ntwb approved these changes Sep 7, 2018

@jeddy3 jeddy3 merged commit 7b1afb4 into stylelint:master Sep 9, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 96.106%
Details
@jeddy3

This comment has been minimized.

Member

jeddy3 commented Sep 9, 2018

  • Fixed: max-empty-lines incorrect line reporting (#3530).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment