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

feat: 570 kotlin support #573

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

Alex-ley-scrub
Copy link
Contributor

@Alex-ley-scrub Alex-ley-scrub commented Nov 8, 2024

/claim #570
fixes #570

todo:

  • Add the language as a supported language in the GritQL grammar
  • Find a tree sitter grammar for the language and add it as a submodule under resources/language-submodules.
  • Add a simple parse test in crates/core/src/test.rs to ensure that the grammar is working.
  • Copy the grammar file into resources/metavariable-grammars. This alternative grammar is used for parsing snippets in GritQL.
  • add resources/add_language.py and run it with python resources/add_language.py --lang kotlin
    • add "kotlin" to edit_grammars.mjs
    • run node ./resources/edit_grammars.mjs kotlin
  • Patch the metavariable grammar to include $.grit_metavariable anywhere we want to substitute a metavariable
  • Add a new language implementation in crates/core/languages/src. This involves implementing the Language trait and adding a new Language enum variant and adding it to all match statements
    • crates/core/languages/src/kotlin.rs
    • cargo test --package marzano-language --lib -- kotlin::tests --show-output
  • Add snippet_context_strings to provide context for snippets to match in.
  • Add kotlin to the PatternsDirectory struct in crates/gritmodule/src/patterns_directory.rs add it to all match statements
  • Add Kotlin to all match statements in crates/lsp/src/language.rs
  • add KOTLIN_LANGUAGE as a static in crates/wasm-bindings/src/match_pattern.rs and add it to all match statements
  • Add test cases for the language in crates/core/src/test.rs. This is a good time to add a few dozen test cases to ensure that the language is parsed correctly, and that the metavariable grammar is working.
    • cargo test --package marzano-core --lib -- test::simple_kotlin --exact --show-output

Greptile Summary

This is an auto-generated summary

Hi! Looks like you've reached your API usage limit. You can increase it from your account settings page here: app.greptile.com/settings/usage

@algora-pbc algora-pbc bot mentioned this pull request Nov 8, 2024
Comment on lines +19 to +27
ALL_LANGUAGES = re.compile(
r"""\/{51}
\/\/ Constants
\/{51}
const allLanguages = (?P<all_languages>\[
(\s*"[\w\-]+",\n?)+
(?P<last_language>\s*"[\w\-]+",)
\])""",
)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use GritQL for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I don't really know GritQL well enough, yet. Whereas regex, I know all too well 🤣

Copy link
Contributor

Choose a reason for hiding this comment

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

Thankfully this one is very simple:

`const allLanguages = [$langs]` where { $langs += `"foo"`}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very nice. Thanks. Might be worthwhile for me to spend a bit of time learning GritQL after this PR then!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm just going to add "At least 10 test cases, including rewrites and metavariables." as you mentioned on the issue description. From your perspective, once I do that (assuming they all pass or are fixed as needed) would this PR be ready to merge? Or have I missed something else?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'd opt to remove this file, since it will potentially require maintenance and doesn't really make things much faster/easier as-is.

@morgante
Copy link
Contributor

morgante commented Nov 9, 2024

Please also add at least one binary/CLI test in tests/apply.rs.

@Alex-ley-scrub Alex-ley-scrub marked this pull request as ready for review November 16, 2024 23:57
@Alex-ley-scrub Alex-ley-scrub requested a review from a team as a code owner November 16, 2024 23:57
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

LGTM

61 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

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.

Kotlin support
2 participants