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

Precedence in the presence of multiple formatting providers #11609

Closed
xaverh opened this Issue Sep 6, 2016 · 24 comments

Comments

Projects
None yet
6 participants
@xaverh
Contributor

xaverh commented Sep 6, 2016

  • VSCode Version: 1.4 and earlier
  • OS Version: Linux and Windows (probably macOS, too)

Given two (or more) "conflicting" extensions (like eg https://marketplace.visualstudio.com/items?itemName=ms-vscode.cpptools and (my own) https://marketplace.visualstudio.com/items?itemName=xaver.clang-format) are installed at the same time.
There is currently no way to decide which extension is allowed to provide the "Format code" feature, which both extensions can provide. Unfortunately, uninstalling either one of them is not a solution because I would like to use the debugging capabilities of the Microsoft C/C++ extension and the formatting of the Clang-Format extension.

@joaomoreno

This comment has been minimized.

Show comment
Hide comment
@joaomoreno

joaomoreno Sep 7, 2016

Member

Is the conflict in the format command? Are there any other conflicts?

Member

joaomoreno commented Sep 7, 2016

Is the conflict in the format command? Are there any other conflicts?

@joaomoreno joaomoreno assigned jrieken and unassigned joaomoreno Sep 7, 2016

@jrieken

This comment has been minimized.

Show comment
Hide comment
@jrieken

jrieken Sep 7, 2016

Member

@xaverh Are you saying there is a conflict between two extensions that both register a formatting edits provider? Iff so, the rules are that the one with the highest score is taken. You guys are likely having the same score in which case the extension that activated later is chosen.

There is not much more from a registration point of view we can do. Your extension can play a dirty trick by depending on the cpp-extension making it become activated later, but there are no optional extension dependencies at this point.

In the end this is a question of how fine granular extension authors built their extensions (one-does-it all extension vs one-extension-per-feature) and a question what extensions a user installs.

Member

jrieken commented Sep 7, 2016

@xaverh Are you saying there is a conflict between two extensions that both register a formatting edits provider? Iff so, the rules are that the one with the highest score is taken. You guys are likely having the same score in which case the extension that activated later is chosen.

There is not much more from a registration point of view we can do. Your extension can play a dirty trick by depending on the cpp-extension making it become activated later, but there are no optional extension dependencies at this point.

In the end this is a question of how fine granular extension authors built their extensions (one-does-it all extension vs one-extension-per-feature) and a question what extensions a user installs.

@jrieken jrieken closed this Sep 7, 2016

@jrieken jrieken added the *question label Sep 7, 2016

@xaverh

This comment has been minimized.

Show comment
Hide comment
@xaverh

xaverh Sep 7, 2016

Contributor

Yes, the question is: Is there any other possiblity (besides the mentioned dirty trick) to choose which formatting feature gets registered? In this case there is a one-feature-extension (which provides formatting for the languages C, C++, Obj-C, Java, and JavaScript) and a one-does-it-all-extension (which provides an allround package for C++, ie formatting, debugging, intellisense). If for any reason I would like to have both installed and use the formatting feature from one specific extension, it would be great to choose which of the two gets ultimately selected (eg by influencing the score or by blocking specific parts of not fine granular extensions).

Contributor

xaverh commented Sep 7, 2016

Yes, the question is: Is there any other possiblity (besides the mentioned dirty trick) to choose which formatting feature gets registered? In this case there is a one-feature-extension (which provides formatting for the languages C, C++, Obj-C, Java, and JavaScript) and a one-does-it-all-extension (which provides an allround package for C++, ie formatting, debugging, intellisense). If for any reason I would like to have both installed and use the formatting feature from one specific extension, it would be great to choose which of the two gets ultimately selected (eg by influencing the score or by blocking specific parts of not fine granular extensions).

@jrieken

This comment has been minimized.

Show comment
Hide comment
@jrieken

jrieken Sep 7, 2016

Member

The problem is real but there are no other solutions to it then what I mentioned above. In contrast to keybindings or snippets language-features like formatters are not registered declaratively. That means, we don't know much of them (like name, containing extensions etc) and they are dynamic meaning an extension can call register/unregister at any time many times.

That only leaves one thing and that is to provide some sort of weight when registering. However, every implementor of a formatter will be naturally biased towards his implementation assigning a weight ala max_value.

So, the only solution I see is to empower the user. He already has control over what extension he installs but it seems that extensions aren't fine grained enough. Another way would be configuration - we ask all competing/conflicting extension to setting ala myExt.enableFormatting: true|false

Last, we could consider a new property in package.json that allows to express optional extension dependencies. We declare the above trick less dirty and your extension would have to list the extensions is wants to have higher priority - it's somehow related to adding a weight but requires more thinking and honesty.

Unsure what the best is, open for suggestions

Member

jrieken commented Sep 7, 2016

The problem is real but there are no other solutions to it then what I mentioned above. In contrast to keybindings or snippets language-features like formatters are not registered declaratively. That means, we don't know much of them (like name, containing extensions etc) and they are dynamic meaning an extension can call register/unregister at any time many times.

That only leaves one thing and that is to provide some sort of weight when registering. However, every implementor of a formatter will be naturally biased towards his implementation assigning a weight ala max_value.

So, the only solution I see is to empower the user. He already has control over what extension he installs but it seems that extensions aren't fine grained enough. Another way would be configuration - we ask all competing/conflicting extension to setting ala myExt.enableFormatting: true|false

Last, we could consider a new property in package.json that allows to express optional extension dependencies. We declare the above trick less dirty and your extension would have to list the extensions is wants to have higher priority - it's somehow related to adding a weight but requires more thinking and honesty.

Unsure what the best is, open for suggestions

@jrieken jrieken added the api label Sep 7, 2016

@xaverh

This comment has been minimized.

Show comment
Hide comment
@xaverh

xaverh Sep 7, 2016

Contributor

How about an option to specify the order of evaluation for extensions:
"extensions.order": ["firstExtension", "*", "secondtolastExtension", "lastExtension"]
where
["*"]
would be the default value and would mean "all other extensions in undefined/default order".

Contributor

xaverh commented Sep 7, 2016

How about an option to specify the order of evaluation for extensions:
"extensions.order": ["firstExtension", "*", "secondtolastExtension", "lastExtension"]
where
["*"]
would be the default value and would mean "all other extensions in undefined/default order".

@jrieken

This comment has been minimized.

Show comment
Hide comment
@jrieken

jrieken Sep 7, 2016

Member

Yeah - along those lines. The challenge is that there is zero knowledge about the extension id when it calls registerXYZ - that is all extensions get the same registerXYZ-method and your proposal would require knowledge about the extension id that is currently running. When requiring the vscode module we would have to patch the extension id into it such that we always know who is doing what.

Member

jrieken commented Sep 7, 2016

Yeah - along those lines. The challenge is that there is zero knowledge about the extension id when it calls registerXYZ - that is all extensions get the same registerXYZ-method and your proposal would require knowledge about the extension id that is currently running. When requiring the vscode module we would have to patch the extension id into it such that we always know who is doing what.

@jrieken jrieken reopened this Sep 7, 2016

@jrieken jrieken removed the *question label Oct 8, 2016

@jrieken jrieken added this to the October 2016 milestone Oct 8, 2016

@jrieken

This comment has been minimized.

Show comment
Hide comment
@jrieken

jrieken Oct 8, 2016

Member

I will tackle this issue during the October release, my current thinking is that we let the user chose a formatting provider when multiple apply. Since we don't know the extension contributing a provider and since 1 extension can contribute multiple providers, I tend towards a solution in which a provider can have a name which is then picked by the user.

  • When calling registerXYZFormattingEditsProvider you can optionally pass a name
  • When the editor calls format and multiple providers apply, let the user chose using the list of names
  • Remember the choice, execute that provider (also on subsequent calls)

Open questions

  • How to handle providers that have no name?
  • How can a user unset his choice? Choice must invalidate itself when extension isn't any longer available etc
Member

jrieken commented Oct 8, 2016

I will tackle this issue during the October release, my current thinking is that we let the user chose a formatting provider when multiple apply. Since we don't know the extension contributing a provider and since 1 extension can contribute multiple providers, I tend towards a solution in which a provider can have a name which is then picked by the user.

  • When calling registerXYZFormattingEditsProvider you can optionally pass a name
  • When the editor calls format and multiple providers apply, let the user chose using the list of names
  • Remember the choice, execute that provider (also on subsequent calls)

Open questions

  • How to handle providers that have no name?
  • How can a user unset his choice? Choice must invalidate itself when extension isn't any longer available etc

@jrieken jrieken changed the title from Precedence in conflicting extensions to Precedence in the presence of multiple formatting providers Oct 8, 2016

@jrieken

This comment has been minimized.

Show comment
Hide comment
@jrieken
Member

jrieken commented Oct 8, 2016

@jrieken

This comment has been minimized.

Show comment
Hide comment
@jrieken

jrieken Oct 10, 2016

Member

fyi @HookyQR - I believe this is interesting for your beautify extension

Member

jrieken commented Oct 10, 2016

fyi @HookyQR - I believe this is interesting for your beautify extension

@HookyQR

This comment has been minimized.

Show comment
Hide comment
@HookyQR

HookyQR Oct 10, 2016

Contributor

Thanks for bringing me in @jrieken. I had request related to this recently where a user wanted to use beautify for HTML and CSS, but not for Javascript. I've added a setting which is used to determine what files beautify should be set up as the formatter for, but that doesn't solve the problem you're facing (it's the inverse really).

I had also been wondering about this kind of thing recently, as there are now at least three extensions that use js-beautify in the back end. If a use had all three installed, how would they even know which one is being used?

One option is to track the registration of the format providers, and when a new one is registered (or the currently selected formatter is removed) ask the user then (or when they first open a matching file). This would also give them the opportunity to not use that function of an installed extension, assuming it provides more functionality.

This would be something like "A new formatter for this type of document has been registered as available ...". Also, if it didn't register with a name, you could use the extension's name to describe it. Then there's the problem that Range edits and Document edits are a different thing.

The uninstall of the 'selected' provided should trigger (when the relevant document type is opened) the same thing, as though no formatter had previously been set.

Storing the selection is a different problem because of the multitude of matching available.

Have fun ;) I'll keep an eye on this thread.

Contributor

HookyQR commented Oct 10, 2016

Thanks for bringing me in @jrieken. I had request related to this recently where a user wanted to use beautify for HTML and CSS, but not for Javascript. I've added a setting which is used to determine what files beautify should be set up as the formatter for, but that doesn't solve the problem you're facing (it's the inverse really).

I had also been wondering about this kind of thing recently, as there are now at least three extensions that use js-beautify in the back end. If a use had all three installed, how would they even know which one is being used?

One option is to track the registration of the format providers, and when a new one is registered (or the currently selected formatter is removed) ask the user then (or when they first open a matching file). This would also give them the opportunity to not use that function of an installed extension, assuming it provides more functionality.

This would be something like "A new formatter for this type of document has been registered as available ...". Also, if it didn't register with a name, you could use the extension's name to describe it. Then there's the problem that Range edits and Document edits are a different thing.

The uninstall of the 'selected' provided should trigger (when the relevant document type is opened) the same thing, as though no formatter had previously been set.

Storing the selection is a different problem because of the multitude of matching available.

Have fun ;) I'll keep an eye on this thread.

@jrieken

This comment has been minimized.

Show comment
Hide comment
@jrieken

jrieken Oct 10, 2016

Member

Thanks for the reply. This is what I have in mind and implemented in PR #13431

  • To keep things simple there will be 1 configuration for all flavours of formatting (document, range, on type)
  • The configuration is a mapping from language to an ordered list of formatter names

The following is an example

"editor.formatter": {
  "json": "superfmt",
  "javascript": ["jsfmt1", "superfmt"]
}

That configuration is used to resort providers. Assume there are two formatters for JSON (superfmt and jsonfmt) and they are score-sorted as [jsonfmt, superfmt]. With the config above the order is reversed to [superfmt, jsonfmt].

What I assume is that once you gave precedence to a formatter you want to use it for all formatting flavours - as long as it supports it. The reverse is that a name from the configuration is ignored when not matching an actual provider, like jsfmt1 can only format files on disk but we have an in-memory file which superfmt can handle.

Member

jrieken commented Oct 10, 2016

Thanks for the reply. This is what I have in mind and implemented in PR #13431

  • To keep things simple there will be 1 configuration for all flavours of formatting (document, range, on type)
  • The configuration is a mapping from language to an ordered list of formatter names

The following is an example

"editor.formatter": {
  "json": "superfmt",
  "javascript": ["jsfmt1", "superfmt"]
}

That configuration is used to resort providers. Assume there are two formatters for JSON (superfmt and jsonfmt) and they are score-sorted as [jsonfmt, superfmt]. With the config above the order is reversed to [superfmt, jsonfmt].

What I assume is that once you gave precedence to a formatter you want to use it for all formatting flavours - as long as it supports it. The reverse is that a name from the configuration is ignored when not matching an actual provider, like jsfmt1 can only format files on disk but we have an in-memory file which superfmt can handle.

@HookyQR

This comment has been minimized.

Show comment
Hide comment
@HookyQR

HookyQR Oct 10, 2016

Contributor

Sounds like a winner to me. Wish I knew about this two weeks ago, would have saved me a bunch of work!

I'm assuming the vscode packaged formatters will get a place in their too.

Contributor

HookyQR commented Oct 10, 2016

Sounds like a winner to me. Wish I knew about this two weeks ago, would have saved me a bunch of work!

I'm assuming the vscode packaged formatters will get a place in their too.

@jrieken

This comment has been minimized.

Show comment
Hide comment
@jrieken

jrieken Oct 10, 2016

Member

Sorry - didn't know until last last week either... Sure thing - the vscode formatter will play nice with this.

Member

jrieken commented Oct 10, 2016

Sorry - didn't know until last last week either... Sure thing - the vscode formatter will play nice with this.

@jrieken

This comment has been minimized.

Show comment
Hide comment
@jrieken

jrieken Oct 10, 2016

Member

fyi @dbaeumer @aeschli this will ask for updates to the language protocol

Member

jrieken commented Oct 10, 2016

fyi @dbaeumer @aeschli this will ask for updates to the language protocol

@HookyQR

This comment has been minimized.

Show comment
Hide comment
@HookyQR

HookyQR Oct 11, 2016

Contributor

Hmm, I see with the 1.6.0 release, the vscode HTML formatter seems to be loaded last. (Definitely after beautify.) So that's fun.

Contributor

HookyQR commented Oct 11, 2016

Hmm, I see with the 1.6.0 release, the vscode HTML formatter seems to be loaded last. (Definitely after beautify.) So that's fun.

@HookyQR

This comment has been minimized.

Show comment
Hide comment
@HookyQR

HookyQR Oct 11, 2016

Contributor

May be because I register when the app opens, and the 'build-in' formatter doesn't load till an HTML file is opened. Side effect of it being pulled out of the app code base proper I guess.

Contributor

HookyQR commented Oct 11, 2016

May be because I register when the app opens, and the 'build-in' formatter doesn't load till an HTML file is opened. Side effect of it being pulled out of the app code base proper I guess.

@dbaeumer

This comment has been minimized.

Show comment
Hide comment
@dbaeumer

dbaeumer Oct 11, 2016

Member

@jrieken how will the new vscode.d.ts API look like ?

Member

dbaeumer commented Oct 11, 2016

@jrieken how will the new vscode.d.ts API look like ?

@jrieken

This comment has been minimized.

Show comment
Hide comment
@jrieken

jrieken Oct 11, 2016

Member

@dbaeumer Unsure yet, but either some sort of identification/name when calling register or a property on the provider. So

registerDocumentFormattingEditProvider(selector, provider, name:string);

or

interface DocumentFormattingEditProvider {

  readonly name: string;

  //...
}
Member

jrieken commented Oct 11, 2016

@dbaeumer Unsure yet, but either some sort of identification/name when calling register or a property on the provider. So

registerDocumentFormattingEditProvider(selector, provider, name:string);

or

interface DocumentFormattingEditProvider {

  readonly name: string;

  //...
}
@dbaeumer

This comment has been minimized.

Show comment
Hide comment
@dbaeumer

dbaeumer Oct 11, 2016

Member

OK. Thanks. Should be easy to adopt then.

Member

dbaeumer commented Oct 11, 2016

OK. Thanks. Should be easy to adopt then.

@jrieken

This comment has been minimized.

Show comment
Hide comment
@jrieken

jrieken Oct 17, 2016

Member

@xaverh @HookyQR fyi - we will discard #13431 and take a different route. In the end we weren't confident with the solution esp that providers have to be named but that you don't know their names until they are active/registered.

Instead, we will add toggles to disable our built in formatters, like typescript.formatter.enable: true|false. This is more simple and better aligned with existing configuration patterns, like clang-format.language.java.enable or javascript.validate.enable. That should make configuration easy and less of a gamble. Your extensions can guide users towards disabling our built-in formatters or use the newish configuration update API to disable them automatically.

Member

jrieken commented Oct 17, 2016

@xaverh @HookyQR fyi - we will discard #13431 and take a different route. In the end we weren't confident with the solution esp that providers have to be named but that you don't know their names until they are active/registered.

Instead, we will add toggles to disable our built in formatters, like typescript.formatter.enable: true|false. This is more simple and better aligned with existing configuration patterns, like clang-format.language.java.enable or javascript.validate.enable. That should make configuration easy and less of a gamble. Your extensions can guide users towards disabling our built-in formatters or use the newish configuration update API to disable them automatically.

@xaverh

This comment has been minimized.

Show comment
Hide comment
@xaverh

xaverh Oct 17, 2016

Contributor

That is, in order to deactivate a formatting feature, the extension itself needs to provide a setting to disable formatting?

Contributor

xaverh commented Oct 17, 2016

That is, in order to deactivate a formatting feature, the extension itself needs to provide a setting to disable formatting?

@jrieken

This comment has been minimized.

Show comment
Hide comment
@jrieken

jrieken Oct 17, 2016

Member

No, it's to disambiguate. Today, VS Code ships with formatters for javascript, typescript, json, and html and when installing your extensions it's not obvious which formatter we use (since there are now two formatters available for javascript and in the end registration time matters). So, when we add toggles to turn off built in formatter the problem doesn't exist anymore

Member

jrieken commented Oct 17, 2016

No, it's to disambiguate. Today, VS Code ships with formatters for javascript, typescript, json, and html and when installing your extensions it's not obvious which formatter we use (since there are now two formatters available for javascript and in the end registration time matters). So, when we add toggles to turn off built in formatter the problem doesn't exist anymore

@xaverh

This comment has been minimized.

Show comment
Hide comment
@xaverh

xaverh Oct 17, 2016

Contributor

In case of JavaScript and TypeScript etc.: Yes.
But for instance in case of C++ this will require the C++ extension developers to provide a setting, if I understand this correctly.

Contributor

xaverh commented Oct 17, 2016

In case of JavaScript and TypeScript etc.: Yes.
But for instance in case of C++ this will require the C++ extension developers to provide a setting, if I understand this correctly.

@jrieken

This comment has been minimized.

Show comment
Hide comment
@jrieken

jrieken Oct 17, 2016

Member

yeah - we will lobby for that to happen

Member

jrieken commented Oct 17, 2016

yeah - we will lobby for that to happen

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.