Respect .prettierignore config #104
Conversation
kapowaz
force-pushed
the
fix-respect-prettierignore
branch
5 times, most recently
from
4d12158 to
d0e37fc
Compare
eslint-plugin-prettier.js
Outdated
| } | ||
|
|
||
| prettier.getFileInfo(sourceFileName, { | ||
| ignorePath: '.prettierignore' |
There was a problem hiding this comment.
I’m not sure if there’s some generic mechanism for exposing default config so that people can specify eslint-plugin-prettier to use a different file than .prettierignore, but given that this was broken until now I’m not sure just how much it matters… maybe one to be addressed in a later PR? 🤷♂️
kapowaz
force-pushed
the
fix-respect-prettierignore
branch
from
d0e37fc to
5237166
Compare
CHANGELOG.md
Outdated
| ## v2.6.3 (2018-07-17) | ||
|
|
||
| * Fix: respect .prettierignore settings ([#88](/~https://github.com/prettier/eslint-plugin-prettier/issues/88)) | ||
|
|
There was a problem hiding this comment.
It's not necessary to update the changelog -- it will be automatically updated during the release.
There was a problem hiding this comment.
Great! I did wonder if that might be automated… 🙈
eslint-plugin-prettier.js
Outdated
|
|
||
| prettier.getFileInfo(sourceFileName, { | ||
| ignorePath: '.prettierignore' | ||
| }).then((sourceFileInfo) => { |
There was a problem hiding this comment.
I don't think this will work -- ESLint rules are required to be synchronous. Does prettier provide a synchronous API for this?
There was a problem hiding this comment.
Yes! I tried both ways so I can update this to use the .sync version 👍
kapowaz
force-pushed
the
fix-respect-prettierignore
branch
from
5237166 to
e7d46c7
Compare
|
Thanks for the comments — I’ve pushed a revised PR, hopefully this should address the issues. The tests still fail due to the issues mentioned in #98, but assuming that can be fixed I think this should be good. |
package.json
Outdated
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "name": "eslint-plugin-prettier", | |||
| "version": "2.6.2", | |||
| "version": "2.6.3", | |||
There was a problem hiding this comment.
Sorry, I should have mentioned this before, but it's also not necessary to update the version in package.json (it will also be automatically updated on release).
eslint-plugin-prettier.js
Outdated
| const prettierSource = prettier.format(source, prettierOptions); | ||
| if (source !== prettierSource) { | ||
| const sourceFileInfo = prettier.getFileInfo.sync(sourceFileName, { |
There was a problem hiding this comment.
Can you add a test for this behavior?
There was a problem hiding this comment.
Can you check for the existence of prettier.getFileInfo.sync before calling it? The plugin needs to continue being able to work for users on old versions of prettier.
There was a problem hiding this comment.
I’ll look into adding a test for this.
As regards checking for the existence of prettier.getFileInfo.sync I could do this, but this functionality is required in order to test for whether a file is being ignored, and so I’ve bumped the dependency to prettier ^1.13.0 for that reason. Is there a reason even future releases of eslint-plugin-prettier needs to always support old versions of prettier?
Maybe this be a minor release instead of a patch release, perhaps?
There was a problem hiding this comment.
Since the user installs prettier independently of eslint-plugin-prettier, it would be a breaking change to drop support for old versions of prettier, because we don't have any control over what version the user has installed.
There was a problem hiding this comment.
I assume that’s an intentional thing, rather than explicitly having this depend on a given version of prettier? I’m not sure it would be possible to add a test to cover checking for a file being correctly ignored if we can’t guarantee a minimum version of prettier is installed though. Would welcome ideas if you think that is possible!
There was a problem hiding this comment.
Yes, the reasoning is that the user might want to choose a specific version of prettier to lint their code with (also see: #1). Admittedly, this was probably more important at the time that pull request was created than it is now, because prettier wasn't yet stable and was making breaking changes more frequently, and people wanted to use the plugin with several different versions.
As far as testing is concerned, I'd be fine with just adding an existence check without directly testing it for now. At some point it might be nice to test on multiple versions of prettier in CI, but that doesn't need to be part of this PR.
There was a problem hiding this comment.
Cool. Like I say this can’t be merged until #98 is fixed anyway, so I guess I can revisit when that is handled (or maybe dive in and have a go at fixing that myself! 😁)
There was a problem hiding this comment.
Sure, that would be great.
kapowaz
force-pushed
the
fix-respect-prettierignore
branch
from
e7d46c7 to
e62f05c
Compare
|
I’ve updated the PR again with some existence-guarding checks against |
|
Closing this PR because the functionality was included in #111. Thanks again for contributing! |
This PR fixes an issue with eslint-plugin-prettier not respecting file ignore patterns specified in
.prettierignore(#88).Currently this is failing due to #98 (since I need to upgrade to prettier 1.13.x to make use of
prettier.getFileInfo()) and so many of the erroneous examples fail with this message: