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

Let processors modify the stylelintResult in case of CssSyntaxError #3063

Merged
merged 5 commits into from Jan 12, 2018

Conversation

5 participants
@emilgoldsmith
Contributor

emilgoldsmith commented Dec 12, 2017

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

resolves #3050

Is there anything in the PR that needs further explanation?

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:

  1. Is there a specific commit format you'd like me to adhere to?

  2. 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 whole stylelint.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.

  3. Since handleError can also throw an error did I get the return value right? This is my first time using flow.

  4. 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.

@emilgoldsmith emilgoldsmith changed the title from Feature/apply resultprocessors on css syntax error to Let processors modify the stylelintResult in case of CssSyntaxError Dec 12, 2017

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Dec 13, 2017

@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?

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Dec 18, 2017

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?

@emilgoldsmith

This comment has been minimized.

Contributor

emilgoldsmith commented Dec 18, 2017

Yep, solves the problem :)

@ntwb ntwb requested a review from CAYdenberg Dec 19, 2017

@ntwb

This comment has been minimized.

Member

ntwb commented Dec 19, 2017

@CAYdenberg Any chance you could review this PR please?

@CAYdenberg

This comment has been minimized.

Contributor

CAYdenberg commented Dec 19, 2017

Yes I can look tonight or tomorrow night.

@@ -33,6 +34,19 @@ const ALWAYS_IGNORED_GLOBS = ["**/node_modules/**", "**/bower_components/**"];
source: string;
}*/
/*:: type resultType = {
config: {

This comment has been minimized.

@CAYdenberg

CAYdenberg Dec 20, 2017

Contributor

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.

// before sending result to user
return stylelint.getConfigForFile(error.file || process.cwd()).then((
result /*: resultType */
) => {

This comment has been minimized.

@CAYdenberg

CAYdenberg Dec 20, 2017

Contributor

You can consider renaming this variable if you agree with my nitpicking, above.

@emilgoldsmith

This comment has been minimized.

Contributor

emilgoldsmith commented Dec 20, 2017

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 :).

@CAYdenberg

This comment has been minimized.

Contributor

CAYdenberg commented Dec 20, 2017

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 createStylelintResult such that you don't need a new convertCssSyntaxErrorToResult function in this file. It's getting late for me though and there might be something I'm missing ... maybe tomorrow night I can play with it a bit if you don't mind?

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.

@emilgoldsmith

This comment has been minimized.

Contributor

emilgoldsmith commented Dec 20, 2017

Yep no worries, take your time :)

@CAYdenberg

This comment has been minimized.

Contributor

CAYdenberg commented Dec 22, 2017

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.

@emilgoldsmith

This comment has been minimized.

Contributor

emilgoldsmith commented Dec 22, 2017

Will do!

(I couldn't seem to make a PR to your repo)

That's weird, I even have the allow maintainers to edit box ticked

@emilgoldsmith

This comment has been minimized.

Contributor

emilgoldsmith commented Jan 2, 2018

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

@emilgoldsmith

This comment has been minimized.

Contributor

emilgoldsmith commented Jan 2, 2018

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! :)

@CAYdenberg

This comment has been minimized.

Contributor

CAYdenberg commented Jan 6, 2018

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!

@@ -58,7 +58,7 @@ const _ = require("lodash");
}
*/
/*:: type resultType = {
/*:: type configForFileT = {

This comment has been minimized.

@ntwb

ntwb Jan 8, 2018

Member

@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?

This comment has been minimized.

@CAYdenberg

CAYdenberg Jan 8, 2018

Contributor

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.

This comment has been minimized.

@ntwb

ntwb Jan 8, 2018

Member

Sweet, thanks for the explanation, and the follow up commit 👍

@emilgoldsmith

This comment has been minimized.

Contributor

emilgoldsmith commented Jan 12, 2018

So this is ready for merging now right?

@CAYdenberg CAYdenberg merged commit 38a4bc3 into stylelint:master Jan 12, 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 decreased (-0.09%) to 95.559%
Details
@CAYdenberg

This comment has been minimized.

Contributor

CAYdenberg commented Jan 12, 2018

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 emilgoldsmith deleted the emilgoldsmith:feature/apply-resultprocessors-on-CssSyntaxError branch Jan 13, 2018

@emilgoldsmith

This comment has been minimized.

Contributor

emilgoldsmith commented Jan 13, 2018

No problem! Thanks for merging it in, I look forward to having the functionality active in our processor :)

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