Skip to content
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

Add support for Magik and PKl languages that are not handled by Linguist #790

Merged
merged 7 commits into from
Jun 11, 2024

Conversation

mmanela
Copy link
Contributor

@mmanela mmanela commented Jun 7, 2024

There are languages which Linguist doesn't plan to handle but we want to still support. This change allows us to define our own list of these languages as a fallback for when linguist (and go-enry) do not know about a language.

Part of https://linear.app/sourcegraph/issue/GRAPH-622/magik-search-filter-support

@keegancsmith
Copy link
Member

Your approach looks correct to me. However, I believe we want to ensure both sourcegraph and zoekt use the same logic for file type identification. In that case it may be fine for us to just import zoekt, but more than likely @varungandhi-src has a better idea. I know he was exploring consistency in language identification a few months ago.

Copy link
Member

@jtibshirani jtibshirani left a comment

Choose a reason for hiding this comment

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

However, I believe we want to ensure both sourcegraph and zoekt use the same logic for file type identification.

@mmanela is making corresponding changes in Sourcegraph too: https://github.com/sourcegraph/sourcegraph/pull/63110. I am +1 to this approach as the best near-term solution.

I don't think we should block this work on pulling out our own language detection library. Although I'd love if we had such a package in the future. And I'd also love for there to be consistency between scip-ctags and our language detection (maybe these could be bundled into the same package?)

internal/languages/language.go Show resolved Hide resolved
internal/languages/language.go Show resolved Hide resolved
Copy link
Member

@jtibshirani jtibshirani left a comment

Choose a reason for hiding this comment

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

Last review comment from me: I'd love if we added a quick check in TestSearchTypeLanguage within index_test.go to exercise this code path!

Then it looks ready to merge -- do you have permissions to do that?

Copy link
Contributor

@varungandhi-src varungandhi-src left a comment

Choose a reason for hiding this comment

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

I think it's OK to have this logic here, since we can change this later easily. I'll see if we can move our language detection logic to a separate repo.

@mmanela mmanela merged commit c21df41 into main Jun 11, 2024
9 checks passed
@mmanela mmanela deleted the mmanela/magikLangSupport branch June 11, 2024 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants