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

Add a compose language #118042

Merged
merged 2 commits into from
Mar 9, 2021
Merged

Add a compose language #118042

merged 2 commits into from
Mar 9, 2021

Conversation

alexr00
Copy link
Member

@alexr00 alexr00 commented Mar 3, 2021

I heard from @bwateratmsft that adding a compose language would be useful for the docker extension, and I think it might make sense. It's easy to try this, so I figured why not. Some of the reasons this could be useful:

  • Extensions can use compose as the language for activation
  • Simplifies when clauses for users who what to scope things
  • Easier to show the proper icon for icon themes
  • Nice to use in files.associations

@alexr00 alexr00 self-assigned this Mar 3, 2021
@alexr00 alexr00 requested a review from aeschli March 3, 2021 14:57
@alexr00 alexr00 added this to the March 2021 milestone Mar 3, 2021
@alexr00
Copy link
Member Author

alexr00 commented Mar 3, 2021

@chrmarti FYI

@bwateratmsft
Copy link
Contributor

bwateratmsft commented Mar 3, 2021

  • Extensions can use compose as the language for activation

I wanted to expand on this some; from telemetry it appears that around 60% of our (Docker extension) activations are due to opening a yaml document, and around 30% are from opening a Dockerfile. Certainly not all of those 60% are actually compose documents (not really sure just how many are), so those would be spurious activations. @alexr00 suggested we could also look at the workspaceContains activation event, which would sorta work, sorta not. Compose documents probably don't get very frequently edited, so I wouldn't necessarily want to activate every time a workspace containing one is opened--there's a good chance it won't be touched in any given session. If it isn't opened, then that would also be a spurious activation.

  • Simplifies when clauses for users who what to scope things
  • Nice to use in files.associations

Users have a lot of variability in file naming for Docker-related stuff. Dockerfiles probably have this more, but compose documents are no exception. Allowing users to adjust files.associations is the easiest way for them to ensure their document is considered a compose file, both for the purposes of everything language-related (highlighting, potential language server features, etc.), but also for all the commands that are scoped to compose docs--for example, our right click > Compose Up command.

This issue has a lot of our reasons for wanting this, and links several issues that we duped against it, in order to gather up everything related. Up until this morning when I spoke to Alex we were tentatively planning on duplicating the yaml grammar straight from VSCode and defining the language, but I was wondering if there was a better way than code duplication--so I asked, and @alexr00 recommended this!

"compose"
],
"filenamePatterns": [
"*compose*.yml",
Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed in the chat, but is this case-sensitive?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have a linux VM to test this on at the moment, but looking at the code it seems that we toLower the pattern so it should be case-insensitive.

// Longest pattern match
if (association.filepattern) {
if (!patternMatch || association.filepattern.length > patternMatch.filepattern!.length) {
const target = association.filepatternOnPath ? path : filename; // match on full path if pattern contains path separator
if (match(association.filepatternLowercase!, target)) {
patternMatch = association;
}
}
}

@@ -13,6 +13,18 @@
},
"contributes": {
"languages": [
{
"id": "compose",
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to disambiguate it as "docker-compose"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems reasonable to me. I can change it.

@aeschli
Copy link
Contributor

aeschli commented Mar 4, 2021

Maybe call the language dockercompose. That would be consistent to language dockerfile. Also *compose*.yml is a bit risky and might too much. I would just go with docker-compose.yml.

@bwateratmsft
Copy link
Contributor

bwateratmsft commented Mar 4, 2021

Also *compose*.yml is a bit risky and might too much. I would just go with docker-compose.yml.

It needs to be more broad. Here's a sampling of a bunch of valid, (mostly) realistic compose file names:

docker-compose.yml
docker-compose.yaml
docker-compose.override.yml
docker-compose.debug.yml
docker-compose.foo.bar.baz.yml
compose.yaml
compose.yml
compose.override.yaml

The list goes on eternally...the only thing I think is reliable is that it's nearly always a yaml extension (yml or yaml), and "compose" appears in the filename nearly always. Sure, you could start to list them off more specifically, but it becomes a maintenance headache.

Something a little less broad that I think would still cover most of our bases:

"filenamePatterns": [
    "compose.yml",
    "compose.yaml",
    "compose.*.yml",
    "compose.*.yaml",
    "*docker*compose*.yml",
    "*docker*compose*.yaml"
]

That would match everything in the list above, but not "composer.yaml" or something along those lines that would (probably) not be a compose document.

@alexr00
Copy link
Member Author

alexr00 commented Mar 9, 2021

Seems like there are no objections, so I'm going to merge this.

@alexr00 alexr00 merged commit 425316c into main Mar 9, 2021
@alexr00 alexr00 deleted the alexr00/composeLanguage branch March 9, 2021 09:52
@github-actions github-actions bot locked and limited conversation to collaborators Apr 23, 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.

None yet

4 participants