fix(eslint): config will not use eslintrc from this project#4
fix(eslint): config will not use eslintrc from this project#4kentcdodds merged 1 commit intomasterfrom
Conversation
Current coverage is 100% (diff: 100%)@@ master #4 diff @@
===================================
Files 2 2
Lines 48 49 +1
Methods 0 0
Messages 0 0
Branches 0 0
===================================
+ Hits 48 49 +1
Misses 0 0
Partials 0 0
|
kentcdodds
left a comment
There was a problem hiding this comment.
Added some notes to the PR to make it easier to see what's going on.
| return { | ||
| '/mock/default-config': { | ||
| rules: { | ||
| semi: [2, 'never'], |
There was a problem hiding this comment.
Before this PR, this was actually inherited via the configuration from this project.
| text: sourceCode, | ||
| eslintConfig: { | ||
| parserOptions: { | ||
| ecmaVersion: 7, |
There was a problem hiding this comment.
This is the reason that you got undefined back. Because ESLint actually failed to parse the code (because it used const).
| const pretty = prettify(text, prettierOptions) | ||
| return eslintFix(pretty, eslintConfig) | ||
| const eslintFixed = eslintFix(pretty, eslintConfig) | ||
| return eslintFixed |
There was a problem hiding this comment.
Made this a variable name just so I can log it easier. Thinking that it could be useful to expose better logging control to log what the text is (if you want a silly amount of logs for example).
There was a problem hiding this comment.
But we'll do that later :)
| function eslintFix(text, eslintConfig) { | ||
| const eslintOptions = { | ||
| // overrideables | ||
| useEslintrc: false, |
There was a problem hiding this comment.
This can be overridden. I don't think that it's advisable to do though or whether this would even be something you'd need to do. But I thought it'd be good to expose a way to override it.
There was a problem hiding this comment.
I think people would want to override this so that eslint can use their local .eslintrc files. Am i missing something here?
There was a problem hiding this comment.
Yeah, I think that makes sense. Normally, I expect people to simply pass the filePath and we'll get the eslintConfig from that (see getConfig below). But if people are providing some special config in addition to what ESLint can find with the .eslintrc then that makes sense :)
| }, | ||
| output: defaultOutput(), | ||
| }, | ||
| { |
There was a problem hiding this comment.
Added this so we don't have trouble in the future :)
|
LGTM 👍 |
9959b96 to
32c2002
Compare
| ] | ||
| }, | ||
| { | ||
| "login": "gyandeeps", |
There was a problem hiding this comment.
Is it ok if I add you as a contributor @gyandeeps? See this for what it'll look like.
There was a problem with the way that eslint fix was applied which basically resulted in test files in this project inheriting from the `eslintConfig` from _this project_ (`prettier-eslint`). So I added `useEslintrc: false` to the config which I believe will fix the issue (can be overridden) Also added a test for the README example so this doesn't happen again!
32c2002 to
e8518fc
Compare
| ] | ||
| }, | ||
| { | ||
| "login": "exdeniz", |
| ] | ||
| }, | ||
| { | ||
| "login": "demoneaux", |
There was a problem hiding this comment.
@demoneaux, are you ok with me adding you to the contributors list? See this for what it will look like.
|
@exdeniz and @demoneaux, I'm going to go ahead and merge this. If you'd like to remove yourself from |
There was a problem with the way that eslint fix was applied which
basically resulted in test files in this project inheriting from the
eslintConfigfrom this project (prettier-eslint). So I addeduseEslintrc: falseto the config which I believe will fix the issue(can be overridden)
Also added a test for the README example so this doesn't happen again!
Could I get a review from @exdeniz and @demoneaux (and anyone else interested? ping @gyandeeps?)
Closes #2