Skip to content
This repository has been archived by the owner on May 30, 2024. It is now read-only.

Add support for HAML syntax highlighting in Heredoc sections #824

Merged
merged 1 commit into from
Jan 2, 2024

Conversation

joevandyk
Copy link
Contributor

@joevandyk joevandyk commented Oct 11, 2023

Motivation

We have support for C++, JS, SQL, HTML, etc syntax highlighting in Heredoc, would be good to have it for HAML as well. Main use case for me is embedding HAML templates inline in ViewComponents.

Implementation

Copy/Pasted the HTML section.

Automated Tests

None, the other Heredoc syntax highlighting don't seem to have any.

Manual Tests

image

@paracycle
Copy link
Member

I am not sure if we should keep adding highlighting for other languages inside Heredocs, since VS Code already does proper syntax highlighting for heredocs, provided a separate extension is installed for syntax highlighting the target language.

For example, compare the following two screenshots from VS Code:
image
image

Since I had the GraphQL syntax highlighting extension installed in my system, VS Code was able to highlight embedded GraphQL query in a heredoc properly, provided the heredoc label is GRAPHQL

@joevandyk
Copy link
Contributor Author

Doesn’t that only work because there’s an entry for heredoc graphql here?

"begin": "(?=(?><<[-~]([\"'`]?)((?:[_\\w]+_|)GRAPHQL)\\b\\1))",
"comment": "Heredoc with embedded GraphQL",
"end": "(?!\\G)",
"name": "meta.embedded.block.graphql",
"patterns": [
{
"begin": "(?><<[-~]([\"'`]?)((?:[_\\w]+_|)GRAPHQL)\\b\\1)",
"beginCaptures": {
"0": {
"name": "punctuation.definition.string.begin.ruby"
}
},
"contentName": "source.graphql",
"end": "^\\s*\\2$\\n?",
"endCaptures": {
"0": {
"name": "punctuation.definition.string.end.ruby"
}
},
"name": "string.unquoted.heredoc.ruby",
"patterns": [
{
"include": "#heredoc"
},
{
"include": "#interpolated_ruby"
},
{
"include": "source.graphql"
},
{
"include": "#escaped_char"
}
]
}
]
},

@davidstosik
Copy link

davidstosik commented Oct 16, 2023

Doesn’t that only work because there’s an entry for heredoc graphql here?

Yes, I think so.

(Prefacing what follows with this: this is only my own understanding on this, for having looked at it from afar, only today. I might be wrong. 😅)

In a virgin VSCode with only the GraphQL extension, I'm not getting GraphQL syntax highlighting:

image

But I get SQL syntax highlighting, because VSCode's out-of-the-box Ruby syntax/grammar declares a "Heredoc with embedded sql":

https://github.com/microsoft/vscode/blob/0c36b92c82064882a228487040187cfc13669c0f/extensions/ruby/syntaxes/ruby.tmLanguage.json#L666-L703

By default, VSCode's Ruby syntax has html, xml, sql, css, c++, c, javascript, "jquery javascript", shell, lua and ruby.
Ruby LSP adds GraphQL and Slim to that list.

@sambostock
Copy link
Contributor

#842 is relevant to this PR. If it's merged first, we'll want to update the test config it adds to add HAML (see #843 for YAML example).

@vinistock
Copy link
Member

@joevandyk let's move forward with this. Can you add a test for HAML using the mechanism introduced in #842? That'll help ensure that it's doing what is expected.

I think just adding HAML to the list is sufficient.

@joevandyk joevandyk force-pushed the jvd/add-haml branch 2 times, most recently from a5522b4 to c2533fb Compare October 20, 2023 18:19
@joevandyk
Copy link
Contributor Author

@vinistock added test!

Anyone know how I can point my local VS Code to use this version of the code, so I can see the HAML syntax highlighting work? Do I build the gem and install it? Or is there a better process?

@vinistock
Copy link
Member

These changes are only in the extension, so you can follow the instructions here. Basically, you need to

  • Make sure dependencies are installed (yarn install)
  • Click F5. This opens a new VS Code window where your local version of the extension is running
  • In the window that just opened, open/create a Ruby file and write a heredoc for HAML to see if its being highlighted

@vinistock
Copy link
Member

Please let us know if it's working as you expected and then we can merge.

Copy link
Contributor

This pull request is being marked as stale because there was no activity in the last 2 months

@github-actions github-actions bot added the Stale label Dec 20, 2023
@joevandyk
Copy link
Contributor Author

it looks good!
image

(similar to how SQL, HTML, JS, etc work)
Copy link
Contributor

@andyw8 andyw8 left a comment

Choose a reason for hiding this comment

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

Most of the team are off for the holidays, but we can hopefully ship this in early January.

One small suggestion: It would useful to add a small mention of this feature to the README.

@github-actions github-actions bot removed the Stale label Dec 22, 2023
Copy link
Member

@vinistock vinistock left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution!

@vinistock vinistock merged commit bcfe678 into Shopify:main Jan 2, 2024
6 checks passed
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.

6 participants