-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Add Go Template and differentiate from Smarty #7687
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
base: main
Are you sure you want to change the base?
Conversation
lildude
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Along with the inline comments, we also need a sample and supporting search string for all extensions you are adding support for. Any one extension that doesn't meet usage requirements will block this PR from being merged until it does so it's best to start small and add support later as usage grows. Users can use an override in the mean time.
| type: markup | ||
| color: "#00ADD8" | ||
| extensions: | ||
| - ".gohtml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the comment at the top of this file. The first extension should be the language's primary extension. Your comments and the fact you've not provided a sample or supporting search result suggests this is not the primary extension for the language.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the reminder, added my reason to the PR description why I did not choose default. If you suggest one particular extension as default based on the description I can update the PR happily.
lib/linguist/languages.yml
Outdated
| - ".tpl" | ||
| filenames: | ||
| - _helpers.tpl | ||
| tm_scope: text.html.go-template |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're guessing and haven't run the tests 😉
This needs a supporting grammar. Linguist doesn't currently have a grammar that provides this scope so this either needs to be set to text (which kinda defeats the point in differentiating when you're not going to get any syntax highlighting) or you need to add a grammar.
I notice you conveniently removed that section of the template too 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure I missed the test, and tried to fix them in f0ffa72. Still I get some repo reference errors on my fork. Do you have any suggestion where should I start with errors like this? Or is it just happening because on a fork?
Rugged::OdbError: object not found - no match for id (7dbcffcf982e766fc711e633322de848f2b60ba5)
Added supporting grammar. Feel free to suggest anything else which might fit better.
Also sorry for the sloppy PR description, I was not expecting such a quick review, and wanted to save my progress.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or is it just happening because on a fork?
Rugged::OdbError: object not found - no match for id (7dbcffc)
This normally happens if you've only included the main branch when forking. We have tests that rely on objects in the test/attributes brach.
|
Thanks for the quick review, tried to fix every issue from the first commit, and added more info to the PR description. Regarding the extensions I researched the topic from every aspect I could, and included search strings. Still if you feel I should remove some, just let me know. For me it looked like every search string has at least 10k results, but I may interpret the search result incorrecty. |
Description
Add Go template as a new language.
This PR is intended to solve #7639 and #6196 so that Helm chart repositories are classified correctly.
Note that Go template use braces similarly to Jinja
{{ }}and the file extension might not differentiate template languages in general. So this PR focus only on Smarty - Go template differences as they usually share the same extension in Helm charts.Rationale for added extensions:
.gohtml: IntelliJ IDEA has association with this extension. (link to source).html.tmpl: GitHub search verifies that is widely used. Mentioned in a StackOverflow answer and also default in a Prettier plugin..gotmpl,.tmpl: Default extensions of Go template files the Go language server (gopls) (link to documentation). Also the VS Code Go Plugin use this default (link to docs)..tpl: Convention used in Helm charts as stated by the Helm documentation ("Conventionally, Helm charts put [...] templates inside of a partials file, usually _helpers.tpl")Since none of the extensions are defined by the Go template documentation, and none of the supporting tools stick to one particular default, I left the list in alphabetical order in the
languages.ymlconfig.Checklist:
I am adding a new language.
_helpers.tplhttps://github.com/search?type=code&q=NOT+is%3Afork+path%3A**%2F_helpers.tpl+%7B%7B-.gohtmlhttps://github.com/search?type=code&q=NOT+is%3Afork+path%3A*.gohtml.gotmplhttps://github.com/search?type=code&q=NOT+is%3Afork+path%3A*.gotmpl.html.tmplhttps://github.com/search?type=code&q=NOT+is%3Afork+path%3A*.html.tmpl.tmplhttps://github.com/search?type=code&q=NOT+is%3Afork+path%3A*.tmpl+%7B%7B-.tplhttps://github.com/search?type=code&q=NOT+is%3Afork+path%3A*.tpl+%7B%7B-#00ADD8I am fixing a misclassified language
I am updating a grammar submodule