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

Support ignore: true in config overrides #15322

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fisker
Copy link
Member

@fisker fisker commented Aug 29, 2023

Description

Not finished yet, just an idea.

{
  overrides: [
    {
      files: ["foo.js"],
      ignore: true,
    },
  ],
},

Closes #14785

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

@WillsterJohnson
Copy link

This doesn't seem like a good direction to go in.

A very good point was raised #14785 (comment) which demonstrates the issue with adding this option.

I think #14785 has already solved the problem which is this PR is attempting to solve. I don't see the benefit of introducing competing PRs, all I can see happening is that there will be further delays in getting the issue solved.

This solution doesn't seem like it will resolve the issues as effectively as #14785 does.

@sosukesuzuki
Copy link
Member

sosukesuzuki commented Sep 15, 2023

I think ignore: true is better than parser: 'ignore' this is because "ignore" isn't the parser name.

For example, if a prettier plugin that support ignored files (e.g. .gitignore, .eslintignore, .prettierignore...) is using "ignore" as a parser name, it will conflict to #14785.

@WillsterJohnson
Copy link

I think ignore: true is better than parser: 'ignore' this is because "ignore" isn't the parser name.

Maybe ignore isn't the perfect name, however not parsing a file is absolutely information that should be gained from the parser option. Perhaps "parser": false would be more appropriate, as currently false is not accepted as a parser name and cannot conflict with an existing package.

Additionally, what should prettier do in this case, and how do we justify disregarding either;
a) the user's demand to use the "foo" parser, or
b) the user's demand to ignore the file

{
  "ignore": true,
  "parser": "foo"
}

This is definitely going to confuse users, especially those who have more complex configurations, as they'll see one of the two and be stuck wondering why it didn't work, having not seen the other. Having conflicting config options like this is not a good idea; it's poor design.

CC @rotu for your thoughts generally, and for "parser": false to replace your "parser": "ignore"?

@sosukesuzuki
Copy link
Member

I think "parser": false is better than "parser": "ignore", but I still prefer "ignore": true. When ignore and parser are specified at the same time, I think Prettier may throw an error as an invalid configuration.

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.

None yet

3 participants