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

Require a prettier config when requireConfig is true #2708

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

redoPop
Copy link

@redoPop redoPop commented Sep 8, 2022

Fix for #2402. When requireConfig is true we cannot rely on the resolvedConfig value to indicate that a Prettier config was found: the resolved configuration may have been picked up from an .editorconfig file instead.

This issue was previously fixed in 8.1.0 but the fix seems to have been undone by subsequent changes.

  • Run tests
  • Update the CHANGELOG.md with a summary of your changes

@ntotten
Copy link
Member

ntotten commented Sep 23, 2022

I don't think this is right. I need to test, but I think configPath is just the location but doesn't necessarily indicate that there is a config at that location.

@redoPop
Copy link
Author

redoPop commented Sep 23, 2022

configPath is the resolution of prettier.resolveConfigFile given the current document path. That's null if no Prettier config file is found in the parent tree, coalesced to undefined in our code.

One thing to watch out for as you test: if you have a prettier config in your home dir or elsewhere in the parent tree then it'll get picked up by resolveConfigFile. That seems like the right behavior for this feature, but it's a potential red herring for testing.

@ntotten
Copy link
Member

ntotten commented Apr 22, 2023

I'll merge this for v10 with prettier 3 as its a breaking change

@github-actions
Copy link

This pull request has been labeled as stale due to inactivity. Reply to keep this pull request open.

@github-actions github-actions bot added the Stale label Jun 22, 2023
@redoPop
Copy link
Author

redoPop commented Jun 22, 2023

This pull request has been labeled as stale due to inactivity. Reply to keep this pull request open.

Keep open for next major release.

@ntotten
Copy link
Member

ntotten commented Jul 10, 2023

Actually, now that i think about this. Why can't you just set the config prettier.useEditorConfig to false? This will prevent the use of editorconfig as the config file and thus will require only a prettier config to be present.

@redoPop
Copy link
Author

redoPop commented Jul 10, 2023

@orenklein gave a good example in #2402: they're working in a monorepo with a global .editorconfig. Some of the individual packages in the monorepo augment those rules with Prettier configs, while others rely entirely on EditorConfig.

My team has a similar use case: we have a boilerplate .editorconfig in every repo to set broad formatting rules, and .prettierrc as an additional layer of configuration for projects that use Prettier.

In both use cases, it's helpful that Prettier allows us to extend EditorConfig rules and establish a hierarchy with a single source of truth for configurations that apply across multiple globs.

A couple of other factors I can think of:

  • I work with projects from multiple teams, and it's useful to know that prettier-vscode will follow another team's Prettier config as best it can – including editorconfig: true – without requiring me to make additional configurations specific to that project.
  • It's caught a few of us by surprise that EditorConfig satisfies the requireConfig rule. That doesn't necessarily make it a bug, but I think the natural expectation is that prettier-vscode requires a Prettier config, not necessarily an EditorConfig.

@ntotten
Copy link
Member

ntotten commented Jul 14, 2023

I think your points are valid, but from the history of this debate I think some people are going to want the current behavior. I don’t love extra complexity, but I wonder is we should evolve this config into maybe three values.

useEditorConfig: “never” | “fallback” | “always”

I am leaning toward not using editor config as a default with the require config, but i feel like we should allow for it.

the next challenge will be picking the names of these config in a way that makes sense. Open to suggestions.

@redoPop
Copy link
Author

redoPop commented Jul 21, 2023

A suggestion for config naming: we could change the values for requireConfig instead of useEditorConfig. Something like this:

"requireConfig": "strict" | "inclusive" | "none"

  • strict = Files are formatted only if a Prettier configuration file is present
  • inclusive = Files are formatted if Prettier or EditorConfig configuration files are present
  • none = Formatting is always enabled (default)

We could then leave useEditorConfig as is and let people opt in to the new behavior explicitly while continuing to support people who've come to expect the current behavior.

@github-actions
Copy link

This pull request has been labeled as stale due to inactivity. Reply to keep this pull request open.

@github-actions github-actions bot added the Stale label Sep 20, 2023
@redoPop
Copy link
Author

redoPop commented Sep 20, 2023

Keep open.

@github-actions github-actions bot removed the Stale label Sep 21, 2023
Copy link

This pull request has been labeled as stale due to inactivity. Reply to keep this pull request open.

@github-actions github-actions bot added the Stale label Nov 20, 2023
@redoPop
Copy link
Author

redoPop commented Nov 20, 2023

Keep open.

@github-actions github-actions bot removed the Stale label Nov 21, 2023
Copy link

This pull request has been labeled as stale due to inactivity. Reply to keep this pull request open.

@github-actions github-actions bot added the Stale label Jan 22, 2024
@redoPop
Copy link
Author

redoPop commented Jan 23, 2024

Keep open.

@github-actions github-actions bot removed the Stale label Jan 24, 2024
Copy link

This pull request has been labeled as stale due to inactivity. Reply to keep this pull request open.

@github-actions github-actions bot added the Stale label Mar 25, 2024
@redoPop
Copy link
Author

redoPop commented Mar 25, 2024

@ntotten What's the next step here? Happy to try implementing either of the proposed config changes in this PR, but not sure which you'd prefer.

@github-actions github-actions bot removed the Stale label Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants