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

Make command templates context type aware #2176

Merged
merged 19 commits into from
Jul 23, 2020
Merged

Conversation

bwateratmsft
Copy link
Contributor

Fixes #2168. Template commands are made context-aware, so that different commands can match different contexts.

With the old behavior, the order is as follows:

  1. Look for any template with a match property that matches
  2. Look for any template without a match property
  3. Use the default template

With this new behavior, the order is as follows (more or less the same as above):

  1. Look for any template whose context type matches AND with a match property that matches
  2. Look for any template whose context type matches AND without a match property
  3. Use the default template whose context type matches

If no contextType property is explicitly specified on any template, it is treated as 'all' and will apply in any context. 'legacy' currently matches only 'moby' contexts, and 'new' currently matches only 'aci' contexts (though it is likely there will be more in the future).

TODO: create an issue or PR to update the docs.

@bwateratmsft bwateratmsft requested a review from a team as a code owner July 21, 2020 15:24
@bwateratmsft
Copy link
Contributor Author

bwateratmsft commented Jul 21, 2020

@karolz-ms and I discussed an alternative approach to new / aci / legacy / all:

  1. Only aci and moby would be defined, right now (same as what is defined for ContextType).
  2. An undefined contextType value would be equivalent to "all". Today, it would mean moby or aci, but if a new context type abc appears, it would still match this and work for it.
  3. The existing commands except compose up/down would have contextType undefined (as far as we know they work in all contexts).
  4. The new compose up/down would have contextType undefined to match any new context, and the old compose up/down would have contextType == moby.
  5. Context type could be defined as an array which would be treated as an OR list, i.e. ["moby", "aci"] would mean either moby or aci (today, this is all possible values, but that may not be true in the future). For example, if a new context type abc appears, then a command with contextType == ["moby", "aci"] would not match it, but one with contextType == undefined would match it. To make such a command match with all three you could do ["moby", "aci", "abc"] (or leave it undefined, of course).
  6. Template preference would go similar to the existing match property, preferring, in order:
    1. Settings-defined templates with either match or contextType, where all specified properties match.
    2. Settings-defined templates with neither match nor contextType.
    3. Hard-coded default templates (only one should ultimately match, to avoid prompting).

Copy link
Contributor

@karolz-ms karolz-ms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

src/commands/selectCommandTemplate.ts Show resolved Hide resolved
src/commands/selectCommandTemplate.ts Show resolved Hide resolved
@bwateratmsft bwateratmsft merged commit d7f9027 into master Jul 23, 2020
@bwateratmsft bwateratmsft deleted the bmw/contextcommands branch July 23, 2020 15:43
Dmarch28 pushed a commit to Dmarch28/vscode-docker that referenced this pull request Mar 4, 2021
* Extend cmd customization for context types

* Add unit tests for command template selection
@microsoft microsoft locked and limited conversation to collaborators Oct 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compose up / down commands will not work in ACI context
2 participants