Let processors modify the stylelintResult in case of CssSyntaxError #3063
Conversation
emilgoldsmith
changed the title
|
@emilgoldsmith Thanks for picking this one up. @stylelint/core Does someone more familiar with flow want to comment on the flow queries. And does anyone have a moment to review the engine/api changes and comment on the related queries? |
|
Thanks, @emilgoldsmith. Good PR, and good questions. Unfortunately, I'm not that familiar with the base of stylelint and can't answer any of this questions :( Have you tried your PR on styled-components/stylelint-processor-styled-components#154? Does it solve the problem? |
|
Yep, solves the problem :) |
|
@CAYdenberg Any chance you could review this PR please? |
|
Yes I can look tonight or tomorrow night. |
lib/standalone.js
Outdated
| @@ -33,6 +34,19 @@ const ALWAYS_IGNORED_GLOBS = ["**/node_modules/**", "**/bower_components/**"]; | |||
| source: string; | |||
| }*/ | |||
|
|
|||
| /*:: type resultType = { | |||
| config: { | |||
There was a problem hiding this comment.
I think the name resultType here is confusing as there is also a stylelint$result used in the same file (and it's a completely different thing?)
If I understand the code correctly this is the result of getConfigForFile ... so is "ConfigForFileT" maybe a more appropriate name?
I'd prefer using T instead of Type (and CamelCase beginning with UpperCase) as it's consistent with CssSyntaxErrorT.
lib/standalone.js
Outdated
| // before sending result to user | ||
| return stylelint.getConfigForFile(error.file || process.cwd()).then(( | ||
| result /*: resultType */ | ||
| ) => { |
There was a problem hiding this comment.
You can consider renaming this variable if you agree with my nitpicking, above.
|
Thanks for the review @CAYdenberg. It seems you missed my initial PR message where I asked a few questions and among other things mentioned that the type I used there was copy pasted from another file (I think createStylelintResult), including the name. If you could address the 4 questions above that would be great! Thanks! And of course if we end up not DRYing it up and just use the copy pasted definition with the more appropriate name you suggested I can also easily handle that, I'll just let you get back to me, now that you have more information, first :). |
|
Oh I see. I did miss that. I guess my critcisms really apply to the original code in createStylelintResult. Lol - it still seems to me that resultType is not really describing a result. I'm generally in favour of DRY code, and I think it would be possible to sufficiently generalize Don't worry too much about commit formats as we squash on merge anyway. (3) This looks fine ... throw within function doesn't affect the return value type. (4) Have to think about this one - maybe will discover something during my other investigation. |
|
Yep no worries, take your time :) |
|
Hey Emil checkout stylelint/issue-3050 and see what you think. If you like you can merge the commits directly into this PR (I couldn't seem to make a PR to your repo). Or if you like you can steal the basic idea and do your own version of it. |
|
Will do!
That's weird, I even have the allow maintainers to edit box ticked |
|
I'll get to this very soon, on the way home for Christmas I actually looked at it and think it looks great, I just wanted to test it with our custom processor which I think I didn't have time for before my flight was taking off, and then I went mostly offline for the holidays. But yeah will probably just merge in your changes @CAYdenberg and I'll be back with a more thorough update (and commits) in the very near future |
|
Okay, looked through it again and tested it with our code, and it seems to still correctly fix our issue at styled-components/stylelint-processor-styled-components#154, and this looks really nice and DRY @CAYdenberg! Thanks for the help! I've looked through the code and it looks good to me and CI is passing, so I guess it's up to you guys whether you want someone that's not @CAYdenberg to review his changes or if I'm sufficient ;). Look forward to having this live! :) |
|
OK @stylelint/core not sure given #3093 if my review should count since I've now committed, but I guess speak now, or forever hold your peace! |
lib/createStylelintResult.js
Outdated
| @@ -58,7 +58,7 @@ const _ = require("lodash"); | |||
| } | |||
| */ | |||
|
|
|||
| /*:: type resultType = { | |||
| /*:: type configForFileT = { | |||
There was a problem hiding this comment.
@CAYdenberg In my ongoing attempt at learning and understanding Flow better, why the name change here? Also should it be configForFileType or are these not really about making them human-readable?
There was a problem hiding this comment.
I found it confusing when reading originally that this type is declared in a file called createStylelintResult, but this is NOT the type of the result. That is stylelint$result (line 79).
So I looked for another name ... this is the type of the returned value of getConfigForFile (encased in a promise), so configForFile is what it is. Possibly the variable name should have been changed too.
I believe the capital T is used in other places but not here, so I'll change that too.
There was a problem hiding this comment.
Sweet, thanks for the explanation, and the follow up commit 👍
|
So this is ready for merging now right? |
|
Yes, thanks for your work on this @emilgoldsmith - sorry about the delays, we're having some ongoing discussions around our process and code reviews. |
emilgoldsmith
deleted the
feature/apply-resultprocessors-on-CssSyntaxError
branch
|
No problem! Thanks for merging it in, I look forward to having the functionality active in our processor :) |
resolves #3050
I think it's pretty self explanatory, I have a few things I was in doubt about style wise / API wise that I would just like to ask / point out though:
Is there a specific commit format you'd like me to adhere to?
There are a few places I copy pasted from another file, so it therefore wasn't completely straight forward whether to modularize in order to apply DRY, specifically the flow type
resultType, and the wholestylelint.getConfigForFile(...).then(...)block. I thought I'd see what you guys thought as I didn't know exactly which file I would put modularized functions/definitions in etc.Since handleError can also throw an error did I get the return value right? This is my first time using flow.
In line 327 and 336 where I specify file path arguments, I was slightly in doubt where exactly to include defaults and which ones in that case in terms of exactly what's expected as I know that for example if you provide the code from stdin with the CLI there's no file name etc. I went with my gut but I would advise an extra thorough look at that part.