Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

feat(search): Add Syntax Highlighting for Magik language#62919

Merged
mmanela merged 15 commits into
mainfrom
mmanela/GRAPH-621
Jun 6, 2024
Merged

feat(search): Add Syntax Highlighting for Magik language#62919
mmanela merged 15 commits into
mainfrom
mmanela/GRAPH-621

Conversation

@mmanela

@mmanela mmanela commented May 24, 2024

Copy link
Copy Markdown
Contributor

Fixes GRAPH-621

Key notes

  1. Added a reference to a fork of the Magik tree-sitter package since the upstream didn't support rust and was using a newer version of tree-sitter which we are not yet supporting.
  2. Add a highlights query for identifying the items to highlight
  3. Added test cases based on language description on wikipedia, example in tree sitter package and other examples online.
  4. I do not have a way to run Magik programs and I could not find a full language specification, so I am not confident this covers everything.
  5. The tree-sitter grammar is missing support for a couple things in their own examples. Upstream issues filed and added in comments

Links

Following links I used to find about the language

Screenshots

image

Test plan

  • Unit tests
  • Validate UI and ensure it looks appropriate

Changelog

  • Added syntax highlighting for the Magik programming language.

@mmanela mmanela changed the title Initial set of changes to support highlighting Magik Add Syntax Highlighting for Magik language Jun 4, 2024
@mmanela mmanela marked this pull request as ready for review June 4, 2024 20:32
@sebastiaanspeck

Copy link
Copy Markdown

I would also like to say that I am a Magik developer and I have been using this tree-sitter for a few months now in my daily work. We appreciate the feedback you have given us by raising the issues. In my daily work I found out that it covers most cases and only has some edge-cases which need to be fixed. If I need to express the coverage in a percentage, I would say it covers 90%-95% of all of the grammar in Magik.

Since I am one of the contributors of the tree-sitter-magik, please let us know if we could help you to integrate this any easier into sourcegraph.
When can we expect to upgrade sourcegraph to the new core tree-sitter? That would make our current grammar work even better for you out-of-the-box, besides not having the generated Rust files.

@mmanela

mmanela commented Jun 5, 2024

Copy link
Copy Markdown
Contributor Author

@sebastiaanspeck Thanks for the quick work there. I pulled the changes in and they work great!

@mmanela mmanela changed the title Add Syntax Highlighting for Magik language feat(graph): Add Syntax Highlighting for Magik language Jun 5, 2024

@kritzcreek kritzcreek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We're currently depending on a rust-bindings branch for our fork of the upstream grammar, could we instead have our main be ahead of the upstream so it's easier to see at a glance how they compare when browsing via GH?

@kritzcreek

kritzcreek commented Jun 6, 2024

Copy link
Copy Markdown
Contributor

Code looks good from my side WDYT about the branch management?

We're currently depending on a rust-bindings branch for our fork of the upstream grammar, could we instead have our main be ahead of the upstream so it's easier to see at a glance how they compare when browsing via GH?

Does this change need an entry in the changelog as well?

@mmanela

mmanela commented Jun 6, 2024

Copy link
Copy Markdown
Contributor Author

@kritzcreek Thanks for the feedback. I updated change log and our forked repo as you suggested

@mmanela mmanela requested a review from kritzcreek June 6, 2024 16:26
@mmanela mmanela changed the title feat(graph): Add Syntax Highlighting for Magik language feat(search): Add Syntax Highlighting for Magik language Jun 6, 2024
@mmanela mmanela merged commit 19ef2d9 into main Jun 6, 2024
@mmanela mmanela deleted the mmanela/GRAPH-621 branch June 6, 2024 20:49
(global_reference)
(identifier)
(slot_accessor)
(label)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This one is interesting. Labels are not really variables. However, we don't really have a SyntaxKind in SCIP to mark labels. GoLand does highlight labels differently, using bold in the theme that I have.

We should probably add a ControlFlowLabel syntax kind to SCIP and use that here. I've filed a follow-up issue: https://linear.app/sourcegraph/issue/GRAPH-661/add-syntaxkindcontrolflowlabel-to-scip

@varungandhi-src

varungandhi-src commented Jun 7, 2024

Copy link
Copy Markdown
Contributor

@sebastiaanspeck does Magik have a standard nil/null value? I don't see it in the grammar, but other Smalltalk languages typically have nil (Smalltalk-80, Squeak, Pharo).

@sebastiaanspeck

Copy link
Copy Markdown

@sebastiaanspeck does Magik have a standard nil/null value? I don't see it in the grammar, but other Smalltalk languages typically have nil (Smalltalk-80, Squeak, Pharo).

You could say _unset is our nil/null value

@sebastiaanspeck

sebastiaanspeck commented Jun 7, 2024

Copy link
Copy Markdown

Once the highlights.scm (and other files that originate from tree-sitter-magik) look good, we can backport any changes to the tree-sitter-magik-repo, so feel free to mention me in any follow-up PRs


(procedure (label) @function)

(documentation) @comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
(documentation) @comment
(documentation) @attribute

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This doesn't seem right to me @sebastiaanspeck . Why would this be an attribute?

@sebastiaanspeck sebastiaanspeck Jun 7, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In Magik we have something called a class browser and the documentation will be shown as an attribute of a method or class in that browser. Documentation is always in the form of ## documentation about a method or class, where a comment is just for other developers etc and always in the form of # comment about a line of code.

Comment on lines +120 to +121
"_method"
"_optional"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
"_method"
"_optional"
"_method"
"_not"
"_optional"

@mmanela

mmanela commented Jun 7, 2024

Copy link
Copy Markdown
Contributor Author

Addressed new comments in this PR

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants