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.

Sign up for GitHub

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

Jump to bottom

Zed forces Prettier to opt out of its usual parser inference #11517

Closed
1 task done
billyjanitsch opened this issue May 7, 2024 · 5 comments · Fixed by #11558
Closed
1 task done

Zed forces Prettier to opt out of its usual parser inference #11517

billyjanitsch opened this issue May 7, 2024 · 5 comments · Fixed by #11558
Labels
defect [core label] prettier Prettier tooling support tooling An umbrella label for language tools, linters, formatters, etc

Comments

@billyjanitsch
Copy link

Check for existing issues

  • Completed

Describe the bug / provide steps to reproduce it

When formatting a file using its native Prettier integration, Zed sends a parser option to Prettier based on Zed's understanding of the file's language. Prettier usually infers which parser to use based on the file path, but Zed's behavior causes Prettier to opt out of this inference.

I don't think this is desirable behavior, because it can cause Zed to format files differently than Prettier would when invoked in other ways (e.g., via CLI).

One example is that Prettier special-cases package.json files, formatting them using the json-stringify parser rather than the json parser. But Zed overrides this behavior by forcing Prettier to use the json parser even for package.json files:

stderr: Resolved config: {[...]}, will format file '[...]/package.json' with options: {[...],"parser":"json","filepath":"[...]/package.json"}

This means that Zed's Prettier integration formats package.json differently than Prettier does when invoked via CLI, causing a mismatch whenever a package.json file is edited with Zed.

This could be fixed by teaching Zed about all of Prettier's various special cases, but that seems like a continuous maintenance burden that Zed probably doesn't want to take on.

Alternatively, maybe Zed doesn't need to pass the parser option at all. I think this was necessary prior to #9598, because there was a bug in the integration that prevented the file path from being passed along, preventing parser inference. But with that bug fixed, Prettier should be able to decide on the right parser to use from the file path alone.

Environment

Zed: v0.133.7 (Zed)
OS: macOS 14.4.1
Memory: 96 GiB
Architecture: aarch64

If applicable, add mockups / screenshots to help explain present your vision of the feature

No response

If applicable, attach your ~/Library/Logs/Zed/Zed.log file to this issue.

No response
@billyjanitsch billyjanitsch added admin read Pending admin review defect [core label] triage Maintainer needs to classify the issue labels May 7, 2024
@Moshyfawn Moshyfawn added prettier Prettier tooling support and removed triage Maintainer needs to classify the issue labels May 8, 2024
@SomeoneToIgnore SomeoneToIgnore mentioned this issue May 8, 2024
Merged
@SomeoneToIgnore
Copy link
Contributor

Thank you for noticing.

Changing this indeed cleans up the code and plugin configs: #11558 but has one side-effect: now, there's no opt-into prettier setting for any languages, and with the default, auto, formatting settings, all languages will send prettier formatting requests, get the failure and only then get back to the LSP requests.
Previous code was able to skip prettier requests entirely.

So I wonder, should we still keep some language plugin config for explicing prettier opt-in?
Or, rather, rework the autoformatting defaults and turn them off entirely?

cc @mrnugget as you were touching prettier integration recently too.
I think it's fine for now the way as in the PR, but let me know your concerns.

@JosephTLyons JosephTLyons added tooling An umbrella label for language tools, linters, formatters, etc and removed admin read Pending admin review labels May 9, 2024
@mrnugget
Copy link
Member

Thanks for tagging, @SomeoneToIgnore!

Few thoughts:

  1. I think the analysis is correct and that we should switch to auto, now that we pass the file name along properly.
  2. Does it make sense to add prettier_parse_name to the language settings so that users can overwrite it? I don't think it does, because I bet that setting is often file specific. And I also think you can overwrite it in prettier config too, right? Just throwing this out there.
  3. I think yes, @SomeoneToIgnore, we should make it explicit. How about we make it a boolean instead of the parser name? format_with_prettier or something like that.

@SomeoneToIgnore
Copy link
Contributor

Does it make sense to add prettier_parse_name to the language settings

Good point, will get rid of it and replace with some boolean.
But now, the whole "migration & backwards compatibility" story arises due to that, so I'll have to double check how things are done properly in the plugin realm.

@maxdeviant
Copy link
Member

Thanks for tagging, @SomeoneToIgnore!

Few thoughts:

  1. I think the analysis is correct and that we should switch to auto, now that we pass the file name along properly.
  2. Does it make sense to add prettier_parse_name to the language settings so that users can overwrite it? I don't think it does, because I bet that setting is often file specific. And I also think you can overwrite it in prettier config too, right? Just throwing this out there.
  3. I think yes, @SomeoneToIgnore, we should make it explicit. How about we make it a boolean instead of the parser name? format_with_prettier or something like that.

Does it actually belong at the language level (i.e., in the language config)?

It seems to me that some languages that may have a Prettier formatter could also have other formatters that could be used. And it seems problematic if the language itself governs whether or not Prettier should be used.

Should we make this a language setting instead so that it is user-configurable?

@SomeoneToIgnore
Copy link
Contributor

As a source for inspiration:
image

all options are disabled by default, even for Rust and are configurable through the menu.

SomeoneToIgnore added a commit that referenced this issue May 15, 2024
@SomeoneToIgnore
Autodetect parser name with prettier by default (#11558)
cb430fc 
Closes #11517 

* Removes forced prettier parser name for languages, making `auto`
command to run prettier on every file by default.
* Moves prettier configs away from plugin language declarations into
language settings

Release Notes:

- N/A
osiewicz pushed a commit to RemcoSmitsDev/zed that referenced this issue May 18, 2024
@SomeoneToIgnore @osiewicz
Autodetect parser name with prettier by default (zed-industries#11558)
d9ad5b9 
Closes zed-industries#11517 

* Removes forced prettier parser name for languages, making `auto`
command to run prettier on every file by default.
* Moves prettier configs away from plugin language declarations into
language settings

Release Notes:

- N/A
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
defect [core label] prettier Prettier tooling support tooling An umbrella label for language tools, linters, formatters, etc
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Autodetect parser name with prettier by default zed-industries/zed
6 participants
@billyjanitsch @mrnugget @maxdeviant @SomeoneToIgnore @Moshyfawn @JosephTLyons