Skip to content
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

Process files only supported by plugins #15433

Merged
merged 35 commits into from Sep 25, 2023

Conversation

sosukesuzuki
Copy link
Member

@sosukesuzuki sosukesuzuki commented Sep 19, 2023

Description

Implementation of #15155 (comment)

Closes #15155
Fixes #15079

This PR is based on #15155 and inherits the test files. The implementation has been completely changed based on the review from @fisker.

This updates the snapshots of some existing tests. I think this is fine, what do you think?

Checklist

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory).
  • (If the change is user-facing) I’ve added my changes to changelog_unreleased/*/XXXX.md file following changelog_unreleased/TEMPLATE.md.
  • I’ve read the contributing guidelines.

Try the playground for this PR

@sosukesuzuki sosukesuzuki changed the title Alternative of 15155 Load files only supported by plugins Sep 19, 2023
@sosukesuzuki sosukesuzuki marked this pull request as ready for review September 19, 2023 15:32
@sosukesuzuki
Copy link
Member Author

I've added tests for #15155 (comment) and #15155 (comment).

I'll add more tests for #15155 (comment) and #15155 (comment) (?) tomorrow.

Copy link
Sponsor Member

@fisker fisker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, more tests are good, but totally optional.

@fisker
Copy link
Sponsor Member

fisker commented Sep 22, 2023

Lint is failing.

for await (const pathOrError of expandPatterns(context)) {
if (typeof pathOrError === "object") {
context.logger.error(pathOrError.error);
for await (const { error, fileName, ignoreUnknown } of expandPatterns(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why rename filename to fileName? Filename is one word.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because expandPatterns yields fileName. But I don't have any opinion on this naming. So I'll fix it. Thanks.

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filename is fine when use alone, but when use together with filePath (I don't think filepath it's a word) and fileUrl , it's a little bit odd. So I use fileName sometime.

I'm not a native English speaker, not sure what's the best.

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I use lots of fileUrlOrPath in recent PRs...

@@ -20,15 +20,15 @@ async function* expandPatterns(context) {
continue;
}

const fileName = path.resolve(filePath);
const filename = path.resolve(filePath);
Copy link
Sponsor Member

@fisker fisker Sep 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: I'm waiting for fast-glob v4 to use absolute: true. There are bugs in v3. mrmlnc/fast-glob#371 mrmlnc/fast-glob#379

@sosukesuzuki
Copy link
Member Author

@fisker What do you think about release this as a patch version? We should release this a minor?

@fisker
Copy link
Sponsor Member

fisker commented Sep 24, 2023

Minor should be better, since it's not a bug fix, we don't format them in v2 before, this become a problem only because we removed the plugin search feature.

We don't add much new features recently. Maybe patch is fine too. I'll leave the decision to you.

@sosukesuzuki
Copy link
Member Author

I'll release this as a minor (3.1)

@t-ricci-digit
Copy link

Waiting for 3.1 release, to update my project dependencies

@benmccann
Copy link
Contributor

Is there any ETA for the 3.1 release? We've been keeping the Svelte project templates on v2, but users have upgrading to v3 on their own and keep hitting this. I'd love to see this fix released

yongrenjie added a commit to Urban-Analytics-Technology-Platform/Urban-Analytics-Technology-Platform.github.io that referenced this pull request Oct 27, 2023
Right now, prettier 3.0 doesn't correctly format astro files using the
plugin. This will be fixed in 3.1 but it hasn't been released yet. If
we upgrade it means that we don't have to specify **/*.astro in the
invocation of prettier.

withastro/prettier-plugin-astro#358
prettier/prettier#15079
prettier/prettier#15433
@fisker
Copy link
Sponsor Member

fisker commented Nov 27, 2023

Yes, it will process every file but ignore files can't find a parser.

@julienw
Copy link

julienw commented Nov 28, 2023

Yes, it will process every file but ignore files can't find a parser.

We had to change our config file from https://github.com/firefox-devtools/profiler/blob/2c0cbe6023ac94e16df11748946c1cdbdb4f6e00%5E/.prettierrc.js
to
https://github.com/firefox-devtools/profiler/blob/2c0cbe6023ac94e16df11748946c1cdbdb4f6e00/.prettierrc.js
That is: change files: 'bin/*' to files: 'bin/*.js'.
IMO this makes sense though: we were configuring prettier to use "espree" on all the files in this directory, but some files weren't javascript. The previous prettier version wouldn't parse the .sh files despite this configuration, while the new version does. I think the new behavior is more correct, however this is still a breaking change.

@fisker
Copy link
Sponsor Member

fisker commented Nov 28, 2023

Sorry about that. Glad you figured how to make it work 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[v3] plugins declared in .prettierrc are not loaded on directory
6 participants