fix: don't ignore file extensions defined in .prettierrc overrides

[bob-laz]

Related to: #108

Your suggestion to use the getFileInfo API appears to work, I am seeing file extensions I defined in .prettierrc having their formatting checked as expected.

You were concerned about speed with this approach. I did some rough testing:

Before the change to getFileInfo:

$ time yarn pretty-quick --check
yarn run v1.22.4
$ /Users/z003zkv/code/sf-case-knowledge/node_modules/.bin/pretty-quick --check
🔍  Finding changed files since git revision null.
🎯  Found 166 changed files.
⛔️  Check failed: scripts/removeClassesFromMetadata.js
{165 more files here, excluded for brevity}
✗ Code style issues found in the above file(s). Forgot to run Prettier?
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

real    0m15.466s
user    0m20.561s
sys     0m1.078s

After the change to getFileInfo:

$ time yarn pretty-quick --check
yarn run v1.22.4
$ /Users/z003zkv/code/sf-case-knowledge/node_modules/.bin/pretty-quick --check
🔍  Finding changed files since git revision null.
🎯  Found 166 changed files.
⛔️  Check failed: scripts/removeClassesFromMetadata.js
{165 more files here, excluded for brevity}
✗ Code style issues found in the above file(s). Forgot to run Prettier?
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

real    0m15.621s
user    0m20.658s
sys     0m1.102s

Not the most thorough test but it doesn't look to be too concerning to me? Let me know if you'd like me to do additional testing.

I can add some unit tests for this as well, but wanted to check and see if the approach was alright with you before spending time there.

[azz]

👋 Hey @bob-laz. Change looks good, let's get it shipped! 🚢

A couple of unit tests would be great.

[azz]

I wonder if the ignored property could also be used instead of doing it ourselves:

https://github.com/azz/pretty-quick/blob/master/src/createIgnorer.js

It might be reading the ignore file twice the way you've done it.

[bob-laz]

@azz updated the PR with your suggestion and a pretty basic unit test. Let me know if there's something else you'd like to see tested.

I wasn't able to get it working to use this function instead of the createIgnorer. I did some testing and it didn't seem like paths specified in .prettierignore were returning ignored: true from getFileInfo for some reason. I was still getting ignored: false.

[azz]

I wasn't able to get it working to use this function instead of the createIgnorer. I did some testing and it didn't seem like paths specified in .prettierignore were returning ignored: true from getFileInfo for some reason. I was still getting ignored: false.

That's because you need to set ignorePath:

https://github.com/prettier/prettier/blob/0bb108ee2b719e810dbb8df920d6fde7ee50086c/src/common/get-file-info.js#L29

https://github.com/prettier/prettier/blob/0bb108ee2b719e810dbb8df920d6fde7ee50086c/src/common/create-ignorer.js#L11-L17

[azz]

So it might actually be better to keep the ignore logic as it is, so we don't have to read and parse the ignore file for every file that is being checked. By not passing ignorePath to prettier it will simply not treat any files as ignored, which is perfect.

[azz]

Could you add an option called --no-resolve-config (called resolveConfig with default = false in the code), and pass that into the getFileInfo call? This way if people run into perf issues and don't care about the edge case this solves they can just turn it off. Don't forget to add it to README 😄

Other than that this is good to ship! 🚢

[bob-laz]

Thanks for the review, I'll get these things added!

[bob-laz]

had to add an alias here otherwise when I passed no-resolve-config it was only resolved as a variable named resolve-config instead of resolveConfig

[bob-laz]

You said to default this to false but from your wording it sounded like we wanted resolving the config to be the default behavior and if this is defaulted to false, I would have to negate it in isSupportedExtension.js to determine whether or not to resolve the config, which seemed confusing

[azz]

Yep you got it right.

[bob-laz]

@azz I updated the readme and added the new config property!

[azz]

Thanks!

[azz]

🎉 This PR is included in version 3.0.2 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀