microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
164.83k stars 29.49k forks source link

Make it clear what happens when multiple formatters for one document apply #41882

Closed octref closed 5 years ago

octref commented 6 years ago

Recently prettier added partial Vue support, and prettier-vscode starts to register itself as a Vue formatter. When user presses format, prettier-vscode was being used (instead of the formatter in Vetur).

https://github.com/vuejs/vetur/issues/658

The user should be given the choice to select a formatter for a specific language.

jrieken commented 6 years ago

The user should be given the choice to select a formatter for a specific language.

How? A formatter has no name nor any other means of identification. We can use the extension name/id but that would only work if there is just one. Worst, the choice cannot be remembered because a formatter is registered with a document selector and (in theory) every document (even of the same language*) can be formatted by a different formatter.

We have always recommended extensions to either ship more fine-grained, e.g. have a separate formatter extension (and let the user decide by installing/uninstalling) or to add an option to enable/disable their formatter (more commonly used).

*) A sample is remote/live edit scenarios in which your extension host knows TypeScript files on your disk (file-scheme) and other TypeScript files from somewhere else, e.g. remote-scheme which are formatted by another entity, e.g. not the TS extension

octref commented 6 years ago

prettier-vscode did add a config prettier.disableLanguages so this can be worked around. So I guess it's each extension's responsibility to make sure they don't clash with others.

However I still think there should be some visual indication when multiple formatters are being run on the same file. Now the order they are being run seem to be random and users thought the formatters are having bugs.

image

I feel no one would want multiple formatters being run on the same file, with order they can't control.

jrieken commented 6 years ago

I feel no one would want multiple formatters being run on the same file,

We don't run multiple formatters. We pick one and use that. It's just hard to pick one, esp when formatting happens from more subtle gestures like format on paste etc

octref commented 6 years ago

OK, running multiple formatter was a misunderstanding, but user not knowing which formatter getting picked isn't that good either.

If the choice of formatter can't be given to users via a setting easily, how about showing a message like "Multiple formatter is available for this document. VS Code defaults to use xxx formatter. Please update your formatter setting if you would like to choose an alternative formatter"?

jrieken commented 6 years ago

VS Code defaults to use xxx formatter.

Problem is the xxx because when an extension registers multiple formatters that are only available under certain condition... It's an uncommon scenario not something we should keep in mind

amatiasq commented 6 years ago

Not sure if this is the place for this question but can't we run formatters sequentially?

jrieken commented 6 years ago

Interesting idea @amatiasq... Sequentially maybe but without restart because otherwise you might format endlessly.

usernamehw commented 6 years ago

How about at least adding a way to get the current formatter(extension name - the one that will be used when you execute Format Document) ?

RiFi2k commented 6 years ago

So let me ask you this, I've read the formatting extensions best practices, understand how it works. But in the case of my extension, which formats the HTML code inside of PHP files (which is very important to a large group of people including anyone who devs WordPress).

Now currently I register this the suggested way, but this is more of a secondary type formatting extension, but now if someone wanted to say use phpcbf to format their PHP, now the user is stuck between which to use what to do. Me personally I run a shell command on save to run my phpcbf command and everything works fine.

My only option if I want to truly play nice would be to do something like run my formatting (because I don't consider it to be the primary) in some sort of on save hook, spawn a shell command type situation. Or I would have to add the ability to to my extension for doing everything.

Honestly I feel like it should work more like you register your extension as a formatting OPTION for said language, then have an option for say xyz language formatting where the value is an array of the registered extensions providing the service and you can just sort/remove whatever ones you want. If someone sorts my name higher, run mine first. If I don't show up, don't run it. If nothing is set at all you could basically do what is currently happening and run them in the extension activation order. Provider extension slug is the registered name.

RiFi2k commented 6 years ago

Just in case anyone was wondering I just changed to subscribing to onBeforeSave as to not force users to make a decision about what formatting they want more. Now it just picks up the slack and anyone could enable any other PHP extension that wants to register to the main hook.

octref commented 5 years ago

Recently prettier added HTML support: https://github.com/prettier/prettier/releases/tag/1.15.0

Users didn't know prettier hijacked the HTML formatter, as there is no indication as to which formatter is being used. Many users are opening issues:

https://github.com/Microsoft/vscode/issues/62996 https://github.com/Microsoft/vscode/issues/64446 https://github.com/Microsoft/vscode/issues/65199

jrieken commented 5 years ago

@octref Please to not change the title of this issue. As long as formatters are registered dynamically and as long as one extension can register many even for different documents, it's not feasible to make a user enumerate them.

I you have strong opinion about this then please make a constructive proposal how this should work.

langdonx commented 5 years ago

it's not feasible to make a user enumerate them.

How is not?

Having a bunch of plugins installed because you work on wildly different projects with different needs shouldn't make your experience in any one of those projects suffer.

Having more insight into what happens when someone presses Alt+Shift+F would be wildly beneficial (especially for new users) when configuring your VS Code or your current workspace. As well, it would be wildly beneficial for extension authors like @octref to be able to choose which formatter (or multiple) executes when a user formats something like a .vue file (https://github.com/vuejs/vetur/issues/850#issuecomment-409666042).

I imagine this could be done in an elegant way for users -- the "Select Language Mode" when you click on the language on the bottom bar could have an additional menu item to view currently installed formatters and their priority. Given the names in that menu, I imagine there could be a new setting (user or workspace) that would let you configure a simple array of formatters to run and in which order for each language you care about.

    "formatters": {
        "javascript": ["howardzuo.vscode-esformatter"],
        "html": ["esbenp.prettier-vscode"],
    }

Is this not feasible and really no more complex than other settings in VS Code?

octref commented 5 years ago

Please to not change the title of this issue.

OK. But that was a misunderstanding and is not a solution to the problems raised here. In the initial comment I also said:

The user should be given the choice to select a formatter for a specific language.


As long as formatters are registered dynamically and as long as one extension can register many even for different documents, it's not feasible to make a user enumerate them.

Well, I have only seen formatters registered for specific languages. Different documents can be dealt with either files.association (user) or contributes.languages.filename (ext author).

constructive proposal how this should work.

IMO 95% of the formatters use a language document selector. We should handle the majority case well. I can't find a valid reason a formatter shouldn't register itself by language.

  1. Offer a setting like preferredFormatterByLanguage, which is a 1-1 mapping from langId to extensionId. Let's assume { 'javascript': 'esbenp.prettier-vscode' }:

    • When esbenp.prettier-vscode does not register a formatter, use the current logic to determine a formatter
    • When esbenp.prettier-vscode does register a formatter, use it and ignore all other formatters registered for javascript
  2. In the case user have not specified a preferred formatter for javascript, and multiple formatters are registered, VS Code should show a QuickInput when user uses format on a JS document:

    • A QuickInput is shown with sources of all registered formatters, like "Built-in TypeScript formatter", "Prettier Formatter"
    • Upon selection, a user setting for preferredFormatterByLanguage should be written.
jrieken commented 5 years ago

Well, I have only seen formatters registered for specific languages.

That's only partially correctly, e.g. TypeScript registers for langId: typescript and scheme: file where other formatter don't. That means for some file this formatter isn't available for some it is.

Offer a setting like preferredFormatterByLanguage, which is a 1-1 mapping from langId to extensionId. Let's assume { 'javascript': 'esbenp.prettier-vscode' }:

@octref @langdonx All built-in formatters come with a setting to disable them (e.g. typescript.format.enable). That means if you install prettier and wanna use it for TypeScript you disable the built-in formatter and all is good. How are arrays enumerating extension ids or quick picks showing extension name better? A formatter extension like prettier could even make this config change upon first use.

langdonx commented 5 years ago

@jrieken Simply put, there are more formatters out there than the VS Code default and Prettier.

Problem 1: I have old projects that use Beautify, I work on open source projects that require Prettier, and I prefer ESFormatter. There's a "Disable (Workspace)" option for extensions that will get things straight, but figuring out which extension is doing as it's not entirely clear so it takes more time than it should. Right now, for example, Prettier is "winning" over Beautify for .js files. I'm not sure if it's because I installed Prettier after Beautify, or if it's because they're run alphabetically? Additionally when I use "Disable (Workspace)", it doesn't seem to write to the .vscode directory, so while I can make Extension suggestions there, I'm guessing I can't disable extensions from there to make it easier for developers.

Problem 2: As reported by @octref, an extension might suddenly start formatting a certain file, which can cause grief (not just for the dev, but for you guys with all the bugs that were filed).

Problem 3: Extensions like Vetur, which format .vue files (that contains HTML, JavaScript/TypeScript, CSS/Stylus) have to ship with formatters that handle all of these things instead of using existing formatters that the user probably already has installed.

jrieken commented 5 years ago

@jrieken Simply put, there are more formatters out there than the VS Code default and Prettier.

That's good to know 😃 Well, we always thought that being able to disable the builtin formatters is enough because why would you install 3 JavaScript formatters and go through the hassle of disabling 2 of them. Anyways, it seems that you want 1 formatter to be used but which one that is depends on the project/workspace. Isn't disabling an extension and enabling it per workspace a solution? Also, how likely is it that open source project A that uses prettier accepts a workspace configuration that talks about beautify?

octref commented 5 years ago

All built-in formatters come with a setting to disable them (e.g. typescript.format.enable). That means if you install prettier and wanna use it for TypeScript you disable the built-in formatter and all is good.

Well, the truth is now if I install Prettier and Beautify, when I run Format Document on JS files, I have no idea which one is used and I can't make a choice. Imagine a frustrated user changing prettier.* settings but nothing worked. I think the 8 upvotes on this comment resonated with that:

How about at least adding a way to get the current formatter(extension name - the one that will be used when you execute Format Document) ?


How are arrays enumerating extension ids or quick picks showing extension name better?

This informs a user about the situation. The setting doesn't have to like that, the important thing is VS Code shows a popup telling user multiple formatters are available and they have to choose.

The message can prompt users to disable some formatters too, if you don't want to add this setting.

I'm suggesting that multiple formatters available for the same document is an anomaly for both ext authors and users. It's not what they intend.

jrieken commented 5 years ago

the important thing is VS Code shows a popup telling user multiple formatters are available and they have to choose. [...] I'm suggesting that multiple formatters available for the same document is an anomaly for both ext authors and users.

Fully agreed and I would say the 'fix' is to uninstall one of the formatters. If not we have to prompt in one or the other way which can be borderline annoying as we apparently must ask per file-type and per workspace. So ideally the prompt isn't "What formatter do you wanna use for this file in this folder" but something like "disable formatter A, uninstall formatter A". Then the problem is with formatters that can format all languages - they might only conflict with foo-lang, but not with others. That makes uninstalling/disabling such extensions unwanted. The exception here are "our" formatter for which we know how to disable them (tho that's layer breaking) but ideally an extension does that when I install it, e.g. if I publish a different JS formatter I would make sure that it disables all other formatters that it is aware of (and conflicting with).

Last, we can always add more commands, e.g. instead of 'Format Document' have 'Format Document with Foo Extension' and 'Format Document with Bar Extension' etc etc. However, what should then happen with things like format on type or format on save (those are UI-less)?

Another option (which would require adoption tho) is that formatters upon registration say what setting enables/disables them, e.g. typescript.format.enable. That would aline with our current story (setting!) but would allow for some less-annoying user-prompting because we know what setting to write.

octref commented 5 years ago

something like "disable formatter A, uninstall formatter A". Then the problem is with formatters that can format all languages - they might only conflict with foo-lang, but not with others.

Prettier extension comes with a setting to do that:

{
  "prettier.disableLanguages": [
    "vue"
  ]
}

I'm ok with each formatter needing to implement an enable/disable setting. Would be better if that could be done automatically in VS Code if I give the setting name typescript.format.enable upon registration. However, I feel that setting doesn't fit the current API. Maybe we should just have a "Formatter Guide" with samples so ext authors can adapt their formatters.

jrieken commented 5 years ago

Maybe we should just have a "Formatter Guide" with samples so ext authors can adapt their formatters.

❗️ https://code.visualstudio.com/blogs/2016/11/15/formatters-best-practices#_multiple-formatters

jrieken commented 5 years ago

Would be better if that could be done automatically in VS Code if I give the setting name typescript.format.enable upon registration

How would that work when looking at your prettier-sample? I don't know how this could be inferred from just the extension id?

However, I feel that setting doesn't fit the current API.

Can you explain your feelings?

octref commented 5 years ago

Can you explain your feelings?

Another option (which would require adoption tho) is that formatters upon registration say what setting enables/disables them, e.g. typescript.format.enable.

For that, do you mean something like this?

registerDocumentFormattingEditProvider(
  selector: DocumentSelector,
  provider: DocumentFormattingEditProvider,
  // When provided, VS Code would register the setting and use it to toggle on/off the formatter
  enableSetting: string
)

❗️ https://code.visualstudio.com/blogs/2016/11/15/formatters-best-practices#_multiple-formatters

People might have a hard time finding this info if it's buried deep in an old blog post. We should at least link to it from a formatter sample or guide.

jrieken commented 5 years ago

For that, do you mean something like this?

Kind of, the extension must still define/register the setting using the default mechanism, but when registering it's refers to that setting so that we can toggle it. E.g

registerDocumentFormattingEditProvider(
  {language: 'fooLang', scheme: 'file'}, 
  provider, 
  "fooLand.formatter.enabled" // setting defined via package.json
)

Then, in case of a conflict, we can show a picker ala Disable formatter from fooLang-extension etc, etc

octref commented 5 years ago

Looks good, setting-wise it's the same story as *.trace.server for Language Servers.

jrieken commented 5 years ago

Looks good, setting-wise it's the same story as *.trace.server for Language Servers.

It's somewhat a fine line because most formatters already have settings, e.g TypeScript. We would assume that the setting is a boolean which means it won't work for prettier without a change of their side. Another alternative would be building this is code, e.g a formatter can optionally implement a function like updateEnablement(selector: DocumentSelector, enabled:boolean) which we would call when a user makes a choice.

octref commented 5 years ago

when a user makes a choice.

Do we then save the state of formatter enablement in workspace data? I still think setting makes more sense. User get a clear understanding of what's going on, and they can enable/disable the settings themselves, too.

rebornix commented 5 years ago
registerDocumentFormattingEditProvider(
  {language: 'fooLang', scheme: 'file'}, 
  provider, 
  "fooLand.formatter.enabled" // setting defined via package.json
)

@octref I know it's probably not good that questioning the design proposal without providing a solution ;( but how does putting a setting in the registerDocumentFormattingEditProvider API help with this problem? For extensions which don't provide formatter enablement, this solution doesn't work but if the extension does have a setting (which is required by this API proposal), why can't users write the setting directly?

octref commented 5 years ago

For extensions which don't provide formatter enablement, this solution doesn't work

I guess the idea is to have enablement handled on VS Code side.

why can't users write the setting directly?

When you format a.ts, you find it misformats. Suppose there are 3 formatters registered for TS, you don't know which one is currently being used. The plan is to offer some help. For example, commands such as Find active formatter for current file, List all formatters for current file. Or even one step further, when you run Format Doc on a document with multiple formatters, show a QuickPick and upon a choice, write settings to disable all other formatters.

jrieken commented 5 years ago

I guess the idea is to have enablement handled on VS Code side.

No, the idea is that formatters must adopt this, e.g they should follow: https://code.visualstudio.com/blogs/2016/11/15/formatters-best-practices#_multiple-formatters

jrieken commented 5 years ago

After taking to @octref I have pushed the following changes:

For the future (February?) we consider not doing anything when multiple formatter for a file apply. Instead of picking one, we would challenge the user to disable one formatter, e.g uninstall or update config. No plans for something automagic or for remembering choices etc. For unattended formatting, e.g format on save, nothing would happen when multiple formatter apply.

usernamehw commented 5 years ago

@jrieken, minor note: can it also open/reveal developer tools just like Developer: Inspect Context Keys?

jrieken commented 5 years ago

Sorry, not possible. The formatter lives in a layer that doesn't know about dev tools

renke commented 5 years ago

I think it can make sense to run different formatters in sequence. For instance, I'd love to implement https://github.com/renke/import-sort as formatter (that sorts import statements) that runs before prettier.

Maybe formatters could be given some kind of priority (think z-index for formatters) with something like 100 being the default (for the main formatter) such that others formatters could have a lower or higher priority (depending on the use case).

usernamehw commented 5 years ago

@jrieken Is there any benefit of printing it into the dev console at all? Would it be possible to create an Untitled file and write into it?

jrieken commented 5 years ago

For instance, I'd love to implement https://github.com/renke/import-sort as formatter (that sorts import statements) that runs before prettier.

That is a very cool idea but in our model it shouldn't be a formatter but a code action. Of those there can be plenty and they can also run on save. Check the CodeActionProvider-API and ping @mjbvz for more information.

For formatting we have actually agreed on not allowing multiple formatters (will rename this issue) and on making it more clear what's happening. Today, in the presence of multiple formatter we just pick one and we will make that more explicit. e.g we will prompt the user to pick a formatter, to disable/uninstall.

DanTup commented 5 years ago

For formatting we have actually agreed on not allowing multiple formatters (will rename this issue)

What happens in the case where two extensions register one anyway? Is the user informed there are two?

In Dart there's an "official" formatter that ships in the SDK, which the Dart extension uses. Many users don't like this formatter (it's very opinionated, not customisable, and always indents with two spaces) and some have talked about making their own formatter. Unfortunately there isn't a way for them to integrate that with the language server, so the best way for them to try it out would be to make their own extension that does formatting, but since they'd still want all other functionality from the Dart extension it's not clear how they'd play together.

I saw the recommendation to add settings, but it seems like that's based around disabling built-in formatters. I'm not sure it really works for two extensions providing the formatting (for ex. which extension would "dart.format.enable" disable Dart formatting for?).

jrieken commented 5 years ago

Is the user informed there are two?

Yes, that's the plan. We will show a message telling you that there is a conflict and we won't proceed unless the user resolves the conflict. Resolving means uninstall or disabling conflicting formatters. We recommend formatter to come with a setting and that setting shouldn't be re-used by another formatter, e.g. the "conflict" wrt dart.format.enable must not exist, a setting per formatter must exist.

DanTup commented 5 years ago

That sounds good to me, thanks for the clarification!

jrieken commented 5 years ago

@sandy081 Is there any chance I get a way to open the extensions viewlet showing a few selected extensions?

steven87vt commented 5 years ago

I ran across this thread today. I have a problem where I need Vetur to understand vue syntax but I have configured it to have no formatters activated. I instead configured prettier-vscode to pass formatting to prettier, then to eslint using prettier-eslint and a .eslintrc configuration which matches prettier and js standard together as closely as possible. This way I have vue, js-standard, stylelint and prettier all working together, i know exactly what linting rules are where and VS code formats all my files and my command line linting matches exactly. This only works on save action. The format document action fails because it removes all but 1 formatter when it reassigns the array length in formatActions#formatDocument

When I debug the formatDocument method in VS code, right now it sees two formatters for .vue: Vetur and Prettier. The telemetry statement executes and then the array length is reassigns to 1, removing the sequence and executing Vetur formatter which was configured to have no formatters. I do not know if Vetur formatter configured to prettier would actually pass to Prettier-VS code and pick up all my custom settings my team is expecting and --fixing all my files.

@jrieken Please forgive me, I have been using VS code for all of a few days at this point. Since the list returns a valid set of formatters, and they are already providing a displayName, could we not just have a simple string settings to pick a preference by name given the current editor._domElement.dataset.modeId value? This way we can override the formatter selection process, not change any API everywhere a VS code formatter was written and we have the ability to print the names in the dev console without having to debug the code to find what is going on.

Example:

preferFormatterIfMultiple: [
  {
    vue: 'Prettier - Code formatter'
  },
  {
    javascript: 'Prettier - Code formatter'
  },
  {
    etc...
  }
]
export function formatDocument(telemetryService: ITelemetryService, workerService: IEditorWorkerService, editor: IActiveCodeEditor, options: FormattingOptions, token: CancellationToken): Promise<void> {
...
// Know how often multiple providers clash and (for now)
// continue picking the 'first' provider
if (provider.length !== 1) {
/* __GDPR__
  "manyformatters" : {
  "type" : { "classification": "SystemMetaData", "purpose": "FeatureInsight" },
  "language" : { "classification": "SystemMetaData", "purpose": "FeatureInsight" },
  "count" : { "classification": "SystemMetaData", "purpose": "FeatureInsight", "isMeasurement": true }
  }
 */
telemetryService.publicLog('manyformatters', {
  type: 'range',
  language: editor.getModel().getLanguageIdentifier().language,
  count: provider.length,
});

provider.length = 1; <-- destroys my prettier world of happiness... 
}
...

image

image

sandy081 commented 5 years ago

@jrieken

Is there any chance I get a way to open the extensions viewlet showing a few selected extensions?

yes. I can add that ability and let you know.

jrieken commented 5 years ago

Moving this out to March (blame me for ⛷ time). We might wanna tackle this more carefully, tho. Things that I have learnt already

jrieken commented 5 years ago

Next steps: know what extensions collide most often and make sure they unregister when disabled

VishalMadhvani commented 5 years ago

This is an interesting discussion so I just wanted to add my opinion as a user.

I think it's valid to have multiple formatter extensions installed and enabled as a user can be working on multiple projects that have different formatting preferences. Expecting a user to have to reconfigure their environment every time they switch I think is not an ideal experience.

I can't speak on the current implementation or complexity of improving the existing behaviour however I would expect:

DanTup commented 5 years ago

Expecting a user to have to reconfigure their environment every time they switch I think is not an ideal experience.

If I understand correctly, you'll be able to disable formatters using Workspace (not just User) settings, so you won't need to reconfigure, and you can commit the .vscode/settings.json file for it to persist with the project.

(That said, a user can't override them - I believe that's what https://github.com/Microsoft/vscode/issues/40233 is asking for - it would be nice to be able to have both "committed/shared workspace settings" and "user-specific workspace settings").

jrieken commented 5 years ago

When having multiple formatters the command will now show quick pick so that you can choose a formatter and/or configure the extension contributing the formatter. For silent formatting like format on paste or format on save we will likely show a status bar message and not format.

Mar-21-2019 15-14-57

Notice the different actions: 'Format Document...' and 'Format Document'. The same applies to format selection.

jrieken commented 5 years ago

I have also pushed changes so that "format on paste" and "format on save" doesn't do anything when multiple formatter apply. This is up for discussion: we can add a message about nothing happening, we can pick some formatter and announce that in the status bar (no control what formatter tho)...

steven87vt commented 5 years ago

Our project integrates prettier, eslint (js standard), and stylelint together through vscode-prettier. The entire system is dependent on the format running eslint --fix onSave. When something like vetur doesnt unregister and collides with prettier this change would cause our entire team based formatting system to fail. What version of vs code do these changes apply to?

jrieken commented 5 years ago

What version of vs code do these changes apply to?

The next version, e.g todays insiders and the 1.33 stable release. Formatters that don't unregister when being disabled are tracked here: https://github.com/Microsoft/vscode/issues/70314. Vetur will ship a fix this milestone