Symbol search: support content-based lang detection #60626
Conversation
| @@ -120,6 +127,10 @@ func makeSearchCondition(column string, regex string, isCaseSensitive bool) *sql | |||
| return sqlf.Sprintf(column+" REGEXP %s", regex) | |||
| } | |||
|
|
|||
| func makeLangCondition(lang string) *sqlf.Query { | |||
There was a problem hiding this comment.
One thing to be aware of: the languages stored here are produced by ctags, whereas the lang filters are expected to be normalized languages from go-enry. I looked through universal-ctags --list-languages and they seem to match up well, so I don't expect this to be a problem in practice.
There was a problem hiding this comment.
Strong suggestion: Please let's use different types here EnryLanguage and CtagsLanguage. These kinds of problems come up in edge cases. For example,
CUDAis all upper-case in ctags butCudain enry.COBOLis uppercase in enry butCobolin ctags.MatLabin ctags isMATLABin enry.Protobufin ctags isProtocol Buffersin enry- ctags has both
Perl6andRaku(which are the same language IIUC) whereas enry only hasPerl 6 ObjectiveCin ctags isObjective-Cin enryAsmin ctags isAssemblyin enry (there's alsoUnix Assembly)- ctags has
ZshandShwhereas enry hasShell.
Some of these are fixable with the case conversion below. But some are not.
I'm not expecting this PR to have a perfect conversion function. But it seems conceptually wrong to conflate these two.
There was a problem hiding this comment.
Thanks, there are indeed more exceptions than I realized. To check I understand your suggestion -- we should have an explicit conversion from go-enry language to ctags language, and update naming to make this distinction clear.
There was a problem hiding this comment.
To check I understand your suggestion -- we should have an explicit conversion from go-enry language to ctags language, and update naming to make this distinction clear.
Yes, and specifically for naming, I'm emphasizing that we should have different types (not just variable names) for these two, and a conversion function from one type to another.
We have something similar in the syntax highlighter, albeit more complicated, to translate Sublime grammar names to Tree-sitter language names.
Right now, the langInclude and langExclude in various places are just strings instead of being domain-specific types, and that's understandable, but changing that to be a more specific type would be valuable. (However, I'm not suggesting you do that in this PR specifically.)
There was a problem hiding this comment.
Great, that matches my understanding -- I will punt on any new types for this PR.
| Repo: api.RepoName(repo), | ||
| CommitID: api.CommitID(commit), | ||
| Query: "", | ||
| IncludeLangs: []string{"Text"}} |
There was a problem hiding this comment.
I thought this patch was additive, so I don't quite understand why this part of the test needs an update, since the "Text" string doesn't appear elsewhere in this patch. Is this test going to continue to pass even if []string{"Text"} is omitted/did you add this to make sure we're exercising the 'has language filter' code path?
There was a problem hiding this comment.
Oops, I did this too quickly! I just pushed a correction. Here I added a lang filter on "text" to ensure the new codepath in internal/rockskip/search.go is exercised. If we remove that logic, this test will fail.
Overall, the rockskip tests are really lacking. If we make further changes, we will invest in new unit testing suites.
|
Thanks for the reviews! I realize this isn't the most high quality + complete solution, but just putting in "best effort" here until we can really invest in completing the feature. |
The symbol service already stores each file's detected language. Now, when the
'search-content-based-lang-detection' feature is enabled, the symbols service
filters directly on the language column.
This PR doesn't update Rockskip, since it doesn't store the detected language.
Relates to https://github.com/sourcegraph/sourcegraph/issues/60341
Test plan
Added unit tests, manual testing