diffs: add starlark file extensions #1012
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -243,6 +243,19 @@ export const EXTENSION_TO_FILE_FORMAT: ExtensionFormatMap = { | |
| py: 'python', | ||
| pyw: 'python', | ||
| pyi: 'python', | ||
| // FIXME: these should be changed to a Starlark file type if/when one | ||
| // appears in shiki. For now, Starlark is highlighted like the Python subset | ||
| // it is. | ||
| BUCK: 'python', | ||
| BUILD: 'python', | ||
| MODULE: 'python', | ||
| PACKAGE: 'python', | ||
| WORKSPACE: 'python', | ||
|
Member
There was a problem hiding this comment. I'm a bit worried about including BUILD, MODULE, and PACKAGE, since those feel pretty generic. The other ones definitely seem bazel related. What do you think about removing those three since there's a custom API you can use to extend for the more ambiguous ones? Another possibility might be to say, what if we just remove all of these direct filenames, until i can improve that part of the system to properly track BUCK files in a sub directory?
Author
There was a problem hiding this comment. Some thoughts: Bazel is super popular, so BUILD is most likely always going to be Bazel, and false positives are probably worth the benefit for these vs the possibility to have a false positive. Approximately one MODULE file appears in a Bazel repo; so it's lower value compared to BUILD. PACKAGE is a buck specific thing and is, of the options, the least credible just given that Buck is a lot less popular than Bazel (c.f. the time i tried to add it to linguist lmao). This is the one I would put into consumers rather than the defaults; I guess the other thought I have is that the consumers could override the decision if they don't like the defaults for BUILD or similar. It's definitely worth distinguishing direct filenames from extensions in the recognizer. |
||
| bazel: 'python', | ||
| bxl: 'python', | ||
| bzl: 'python', | ||
| star: 'python', | ||
| sky: 'python', | ||
| qml: 'qml', | ||
| qmldir: 'qmldir', | ||
| qss: 'qss', | ||
|
|
||
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.
When a Bazel/Buck file is not at the repository root, this exact-filename entry is never used: parsed diffs keep directory prefixes (for example
packages/diffstests snapshot names likepackages/diffs/src/index.ts), andgetFiletypeFromFileNameonly checksEXTENSION_TO_FILE_FORMAT[fileName]before falling back to extensions. As a resultgetFiletypeFromFileName('src/BUILD')andgetFiletypeFromFileName('foo/BUCK')still returntext, so the common nested package files this change is trying to support won't be highlighted unless they use.bazel/.bzl.Useful? React with 👍 / 👎.
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.
Yeah this issue is due to my laziness around our file name detection (i.e. i need to probably extend this to properly test the filename), but probably shouldn't be apart of this PR.