Fix reported line numbers #140

chinesedfan · 2017-11-05T09:54:38Z

Fixes #128. And also able to pass #129.

 6:  const Button2 = styled.button.attrs({
 7:    type: 'normal'
 8:  })`
 9:    display: block;
10:    ${
11:       props => props.isHovering && interpolatedStyle
12:    }
13:  color: ${color};
14:    background: blue;
15:  `

Current codes try to generate source map from processedNode.loc.start.line, which is line 6. But it should be line 8.
If the interpolation contains multiply lines, we should make sure it is substituted by same count of lines.

coveralls · 2017-11-05T13:39:57Z

Changes Unknown when pulling 0760875 on chinesedfan:fix_lineno into ** on styled-components:master**.

emilgoldsmith · 2017-11-05T14:20:10Z

At a glance it looks pretty cool, I'll have to take a bit more time later to look into the code more thoroughly though, but for one, since you say it passes #129 what about we just copy paste that test case into this PR then? :). The more tests the better!

chinesedfan · 2017-11-05T14:27:01Z

@emilgoldsmith I just added some unit tests in this PR, but tested locally with #129. My idea is that merging this PR first, and #129 later. After all, #129 belongs to another author. Waiting for your review feedbacks. :)

emilgoldsmith · 2017-11-05T14:28:24Z

My idea is that merging this PR first, and #129 later. After all, #129 belongs to another author.

Yeah sure, that's very respectful and makes sense :)

emilgoldsmith

Hey! Thanks again for your contributions, when you find the time if you could just get back to me on my concerns, if I'm wrong about anything please have patience with me :)

emilgoldsmith · 2017-11-12T19:33:31Z

src/parsers/index.js

@@ -74,7 +74,7 @@ const processStyledComponentsFile = (ast, absolutePath, options) => {
      extractedCSS.push(stylelintCommentsAdded)
      sourceMap = Object.assign(
        sourceMap,
-        getSourceMap(extractedCSS.join('\n'), wrappedContent, processedNode.loc.start.line)
+        getSourceMap(extractedCSS.join('\n'), wrappedContent, processedNode.quasi.loc.start.line)


emilgoldsmith · 2017-11-12T19:40:53Z

src/utils/tagged-template-literal.js

@@ -141,6 +141,17 @@ const interleave = (quasis, expressions, absolutePath) => {
       */
      substitute = '$dummyValue'
    }
+    // Make sure substituted by same count of lines
+    const intentCount = prevText.split('\n').pop().length


By intent you mean indent right? That confused me for so long, can we please change that?

And on another note, I'm pretty sure you have some indentation edge cases here with examples such as:

styled.div` color: ${colorVariable} `

right? Your length here would include all the color: part

And of course adding a test for this as you fix it would also be great!

Yes, it's a typo. intent should be indent.

emilgoldsmith · 2017-11-12T19:44:59Z

src/utils/tagged-template-literal.js

@@ -141,6 +141,17 @@ const interleave = (quasis, expressions, absolutePath) => {
       */
      substitute = '$dummyValue'
    }
+    // Make sure substituted by same count of lines
+    const intentCount = prevText.split('\n').pop().length
+    const intent = new Array(intentCount).fill(' ').join('')


Hmm, I think we might also be making similar mistakes like this other places in the code (I don't quite remember if we handle it perfectly everywhere, such as in our indentation fixers) but would this break some indentation Stylelint rules if someone was using tabs? Adding a test that tests that would definitely be great whether this is a breaking test or not :)

Thanks very much for correcting me! A better way may be selecting all left spaces from the last line, i.e. prevText.split('\n').pop().replace(/^(\s*).*$/, '$1').

emilgoldsmith · 2017-11-12T19:47:07Z

src/utils/tagged-template-literal.js

+    const intent = new Array(intentCount).fill(' ').join('')
+    const targetLines = quasis[i + 1].loc.start.line - quasis[i].loc.end.line + 1
+    let currentLines = substitute.split('\n').length
+    while (currentLines < targetLines) {


Okay, so just to make sure I understand what you're doing here, you're just making sure that we substitute the correct amount of lines in so we don't have propagating line errors further down, yes? As in, we make sure that the length of our processed CSS is the same as the length of their actual TTL (Tagged Template Literal). Right?

Right. But it's a little hard to keep line counts without introducing new styling errors. How about padding lines by /* stylelint-disable-line */?

Hey, that's quite a good idea. I like it. It would make the logic so much easier, just add x empty lines that only say /* stylelint-disable-line */ and we'll be golden, right?

Make sure there aren't any edge cases with the indentation rules / empty lines before blocks etc. that are introduced as I'm not sure exactly how stylelint works with the indentation rules, but yeah I like that idea, the simpler the better!

chinesedfan · 2017-11-19T15:55:23Z

@emilgoldsmith Sorry for my late responses. I am busy these days, so I leave some comments to you first. This feature is really important and I think I will implement it finally. :)

monsieurnebo · 2017-11-22T14:38:39Z

Yes it is :)

coveralls · 2017-11-26T12:46:38Z

Coverage increased (+0.02%) to 99.66% when pulling cbfb316 on chinesedfan:fix_lineno into b864ff3 on styled-components:master.

chinesedfan · 2017-11-26T12:48:07Z

@emilgoldsmith /* stylelint-disable-line */ can only solve problems in comment nodes. I update with a new strategy that recording all interpolation lines first and filtering indentation warnings in those lines.

coveralls · 2017-11-26T12:56:24Z

Coverage increased (+0.02%) to 99.659% when pulling 088e986 on chinesedfan:fix_lineno into b864ff3 on styled-components:master.

emilgoldsmith

Thanks for getting back to us @chinesedfan! And interesting new approach, at first I was a bit skeptical about the modifying of the stylelintResult object, but now that I looked at it thoroughly I think this could actually be made to work quite well. Just a few comments here and there where I think the main one is making sure that we're handling updating the errored key on the stylelintResult object properly

emilgoldsmith · 2017-11-26T18:13:50Z

src/index.js

+      .filter(
+        warning =>
+          // Filter indentation warnings generated by interpolations substitution
+          warning.rule !== 'indentation' ||


I think this would be a lot more readable if you wrote it in the negative way, so the focus is on when we filter it out:

!(warning.rule === 'indentation' && interpolationLines.indexOf(lineCorrection[warning.line]) >= 0)

emilgoldsmith · 2017-11-26T18:16:46Z

src/index.js

-    return Object.assign({}, stylelintResult, { warnings })
+    return Object.assign({}, stylelintResult, {
+      warnings,
+      // Remember to fix


Is this a note to yourself to remember something, or is it an explanatory comment? If it's an explanatory comment maybe make it a bit more verbose, such as // Undo errored if all warnings were caused by interpolation substitutions

emilgoldsmith · 2017-11-26T18:27:24Z

src/parsers/index.js

+      // Save interpolation lines
+      node.quasi.expressions.forEach((expression, i) => {
+        let l = node.quasi.quasis[i].loc.end.line + 1
+        while (l <= node.quasi.quasis[i + 1].loc.start.line) {


Doesn't this throw an error when i === node.quasi.expressions.length - 1 ??

No, quasis.length should be always greater 1 than expressions.length. I learned from similar codes here.

Ahh yes, you're iterating over expressions not quasis. Makes sense, my bad!

It'd be an okay idea to add a comment about that, not super urgent, as it's not that unclear, but no harm in documenting code well!

emilgoldsmith · 2017-11-26T18:39:48Z

src/utils/tagged-template-literal.js

@@ -146,6 +146,14 @@ const interleave = (quasis, expressions, absolutePath) => {
       */
      substitute = '$dummyValue'
    }
+    // Make sure substituted by same count of lines


This looks good

emilgoldsmith · 2017-11-26T18:46:00Z

src/index.js

+    return Object.assign({}, stylelintResult, {
+      warnings,
+      // Remember to fix
+      errored: warnings.length ? true : undefined


This modifying of the stylelintResult definitely makes me a bit nervous in general, but I think your filter is mostly pretty contained so I think I can be okay with it, we need to be really accurate though. Are you sure that this behaviour you wrote here conforms with how Stylelint does it exactly? The only documentation I can find that's the most reliable is these two: https://stylelint.io/user-guide/node-api/ https://stylelint.io/developer-guide/processors/ and this code https://github.com/stylelint/stylelint/blob/master/lib/createStylelintResult.js which may or may not be the object we're passed as I didn't dig deeper into it.

Could you please dig into it deeper and come back with a bit of "proof of correctness" as I could imagine that we may need to check if the rules have severity: 'error' or something like that, though the thing is I'm not sure so just want to make sure we don't screw something up here

This really makes me unhappy here. I found that errored can be undefined/null/true. See stylelint#createStylelintResult.js.

It is hard to recover errored unless authors of stylelint can give us a reasonable explanation in stylelint#3041.

Hmm yes that does seem pretty messy indeed, let's wait a bit and see if you get a reply there, but one thing that I can definitely see from your research (this part in particular) is that we need to be doing a check here whether any of the warnings have severity level "error", but yeah, maybe we also need to check the null and other stuff.

One way would also be to check whether the errored flag is currently set, if it is not we know we don't have to change anything (as we're only removing errors in our modifications so it would never cause an error to show up, only to be removed), if it is set to true though, then check if any of the remaining warnings have severity level "error", and if they don't change errored to undefined I guess right? It should be undefined not false according to your research yeah?

I think this would handle the case where it's null etc. as well, what do you think?

Right, I should check severity levels. Good news is that ignored files will not invoke our processor, so we can do not consider the null case.

Nice :).

Yeah so I think the method I proposed above should work yeah? Don't do anything if it's falsy, if it is truthy check if any warnings with severity level error, and if there aren't any change it to undefined.

emilgoldsmith · 2017-11-26T18:48:46Z

src/parsers/index.js

      )
+      // Save interpolation lines
+      node.quasi.expressions.forEach((expression, i) => {
+        let l = node.quasi.quasis[i].loc.end.line + 1


I was actually just about to comment that maybe we should not include the first line, but that's actually also what you're doing here right? The +1 is excluding the line where the interpolation starts yeah? So if I for example did a single line interpolation ${someVariable} I would still be checked for indentation errors yeah? If so all good and good on you for considering it as well

Right, the indentation warning caused by the first line of the interpolations will not be filtered. Because indentations are wrote by users themselves, instead of generated by dummy comments padding.

Yep, I agree that's exactly the behaviour we would want.

I am thinking about renaming interpolationLinesMap as another name. How about dummyCommentsMap or something else?

Hmm yeah, that's probably a fine idea to rename. So the purpose is to save where we have put in space filling comments in interpolation substitutions right... I feel like we should be able to come up with something better/even more specific than dummyCommentsMap, I guess it's also not really even a mapping right? More like a list. How about dummyInterpolationLines? Or dummyInterpolationLinesList?

This is definitely a subjective issue though, no real answer to this.

chinesedfan · 2017-12-01T13:47:55Z

@emilgoldsmith Comments mentioned above have been handled. I gave up renaming the variable because naming is really the hardest problem in programming. The current interpolationLines is not so bad.

coveralls · 2017-12-01T13:52:53Z

Coverage increased (+0.02%) to 99.663% when pulling 14d313e on chinesedfan:fix_lineno into b864ff3 on styled-components:master.

emilgoldsmith

Looks great!

Let's just squash these commits into like 1-3 commits though yeah? Whatever you feel creates a good history, I'm okay with 1, but if you feel there's a natural way to split it into several commits to look nicer on your Github graph I'm okay with that as well haha ;).

emilgoldsmith · 2017-12-01T19:23:40Z

src/index.js

-    return Object.assign({}, stylelintResult, { warnings })
+    const result = Object.assign({}, stylelintResult, { warnings })
+    // Undo `errored` if no warnings with error severity any more
+    if (result.errored && !warnings.some(warning => warning.severity === 'error')) {


Really nicely and succinctly written :)

chinesedfan · 2017-12-02T04:33:54Z

@emilgoldsmith Haha, my Github graph doesn't matter. You can simply select "squash and merge". Thanks for your sweet reviews during these days!

emilgoldsmith · 2017-12-02T04:50:49Z

No problem haha!

Actually "squash and merge" isn't enabled for this repo, so yeah if you find time to squash it at any point we can merge it right in :)

chinesedfan · 2017-12-02T05:19:22Z

@emilgoldsmith I rebased and squashed as 3 commits.

coveralls · 2017-12-02T05:21:41Z

Coverage increased (+0.02%) to 99.663% when pulling ca14148 on chinesedfan:fix_lineno into b864ff3 on styled-components:master.

emilgoldsmith · 2017-12-02T05:28:51Z

Awesome. Thanks again for the hard work and several revisions! This is a nice addition to the repo :)

chinesedfan · 2017-12-03T04:21:25Z

@emilgoldsmith Last question, what's the publish strategy of this repository? I want to know when I can use the version with this fix. 😄

emilgoldsmith · 2017-12-03T05:24:55Z

I'll publish it right now :). No defined strategy, more like the three publishers publish whenever they see fit / new changes are merged in

emilgoldsmith · 2017-12-03T05:32:48Z

Done :). Update to 1.2.0 and you'll have your changes

chinesedfan · 2017-12-03T09:59:52Z

Great! I will upgrade it in my project and keep observing.

chinesedfan mentioned this pull request Nov 5, 2017

Integrate styled components checking tasks gitpoint/git-point#604

Closed

emilgoldsmith suggested changes Nov 12, 2017

View reviewed changes

emilgoldsmith suggested changes Nov 26, 2017

View reviewed changes

emilgoldsmith approved these changes Dec 1, 2017

View reviewed changes

chinesedfan added 3 commits December 2, 2017 13:04

Fix reporting the wrong line numbers

18f1289

Fix substitution changes line counts

9cce4c8

Filter indentation warnings generated by interpolations substitution

ca14148

chinesedfan force-pushed the fix_lineno branch from 14d313e to ca14148 Compare December 2, 2017 05:15

emilgoldsmith merged commit b5b9612 into styled-components:master Dec 2, 2017

emilgoldsmith mentioned this pull request Dec 2, 2017

initial add of breaking test suite #129

Merged

chinesedfan mentioned this pull request Jan 25, 2018

Version 1.2.0 breaks complex multiline expressions #165

Closed

Fix reported line numbers #140

Fix reported line numbers #140

Conversation

chinesedfan commented Nov 5, 2017

coveralls commented Nov 5, 2017

emilgoldsmith commented Nov 5, 2017

chinesedfan commented Nov 5, 2017

emilgoldsmith commented Nov 5, 2017

emilgoldsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinesedfan commented Nov 19, 2017

monsieurnebo commented Nov 22, 2017

coveralls commented Nov 26, 2017

chinesedfan commented Nov 26, 2017

coveralls commented Nov 26, 2017

emilgoldsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinesedfan commented Dec 1, 2017

coveralls commented Dec 1, 2017

emilgoldsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinesedfan commented Dec 2, 2017

emilgoldsmith commented Dec 2, 2017

chinesedfan commented Dec 2, 2017

coveralls commented Dec 2, 2017

emilgoldsmith commented Dec 2, 2017

chinesedfan commented Dec 3, 2017

emilgoldsmith commented Dec 3, 2017

emilgoldsmith commented Dec 3, 2017

chinesedfan commented Dec 3, 2017 • edited Loading

chinesedfan commented Dec 3, 2017 •

edited

Loading