feat(search): Support Magik language file filter in search #63110
Conversation
…support for Magik (and other languages in the future)
mmanela
changed the title
…support for Magik (and other languages in the future)
…ourcegraph/sourcegraph into mmanela/GRAPH-622-magik-filter
jtibshirani
left a comment
jtibshirani
left a comment
There was a problem hiding this comment.
@mmanela and I chatted offline about adding more tests.
We also decided that this PR will only be scoped to indexed/ unindexed search. In follow up work, we will handle
- Symbol search (both symbols and Rockskip)
- Other places that use
enrydirectly, specifically Code Insights
| * Languages that are not present in the go-enry library | ||
| * and so are not in go-enry either |
There was a problem hiding this comment.
You could changes this so that:
- The first mention of go-enry is either Linguist
- OR you could changes this to say something like "Languages not present in the go-enry library, which we use for language detection in the backend"
| // Not in Linguist and they are not likely to add | ||
| 'Magik', |
There was a problem hiding this comment.
Linking the PR would've been useful here as context.
| // getLanguagesByExtension is a replacement for enry.GetLanguagesByExtension | ||
| // It supports languages that are missing in go-enry | ||
| func GetLanguageExtensions(alias string) []string { | ||
| if lang, ok := unsupportedByEnryNameToExtensionMap[alias]; ok { | ||
| return []string{lang} | ||
| } | ||
|
|
||
| return enry.GetLanguageExtensions(alias) | ||
| } |
There was a problem hiding this comment.
If you look just below this, we have a function which overrides the behavior of enry's GetLanguagesByExtension. However, the behavior of this function GetLanguageExtensions is not consistent with that, because it doesn't make use of the data in overrideAmbiguousExtensionsMap.
Specifically, if getLanguagesByExtension("foo.json") returns "JSON", then it is not OK for GetLanguageExtensions("OASv2-json") to return ".json" or "json" (https://sourcegraph.com/github.com/github-linguist/linguist/-/blob/lib/linguist/languages.yml?L4750-4760)
| var unsupportedByEnryAliasMap = map[string]string{ | ||
| // Pkl Configuration Language (https://pkl-lang.org/) | ||
| "pkl": "Pkl", | ||
| // Magik Language |
There was a problem hiding this comment.
This comment does not convey any extra information over the line of code below it; it would make sense to either add a link to help the reader or not have a comment at all.
| // NOTE: Add to linguist on 6/7/24 | ||
| // can remove once go-enry package updates | ||
| // to that linguist version |
There was a problem hiding this comment.
Leaving this purely as a code comment increases the likelihood that it will be forgotten. Instead, you could've added a Go test that the enry list of languages doesn't have this one by doing a map lookup in https://pkg.go.dev/github.com/go-enry/go-enry/[email protected]/data#LanguageInfo
In the code, you can add an easy-to-search for marker, like TODO(id: remove-pkl-special-case) and mention that in the test code so that when someone sees the test failure, they know what other places need updating.
Fixes GRAPH-622
Adds support to the lang file filter for Magik language (
lang: Magik). In order to do that we add wrappers around go-enry and update search code to use them. This provides flexibility for us in the future to support other languages that are not in Linguist as well.This PR does NOT update symbols for Magik (that needs the future scip-ctags work) or udpated code insights (will follow up with that)
Test plan
Changelog
lang:magik)