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

Add syntax highlighting for Pkl files #61478

Merged
merged 28 commits into from
Apr 4, 2024
Merged

Add syntax highlighting for Pkl files #61478

merged 28 commits into from
Apr 4, 2024

Conversation

mmanela
Copy link
Contributor

@mmanela mmanela commented Mar 28, 2024

Fixes PRIV-2879

Adds syntax highlighting for the Pkl configuration language.

This does NOT add pkl files to the lang: filter.

Key Changes/Notes/Questions

  1. Added reference to the Pkl tree-sitter package
  2. Added references to the new language in the syntax highlighter rust project
  3. Added the Pkl tree sitter queries (.scm files) (got them from the pkl tree sitter repo)
  4. Add reference to pkl files in gosyntect/langauges.go and highlight/languages.go to ensure it is picked up properly since pkl does not exist in go-enry.
  5. Generated unit test snapshot for Pkl

image

What about updating the Docs?

If we agree these are the minimal steps to add a highlighter, I can update the docs with these steps. The one wrinkle is handling if the given language exists in

  • Syntect
  • Enry

If they do or don't exist in those places some of the steps may change a tiny bit I think.

Test plan

  • Update unit tests
  • Validate local instance colors pkl correctly
  • validate in S2 after deploy

Copy link
Member

@eseliger eseliger left a comment

Choose a reason for hiding this comment

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

I don't feel confident to approve this, but I don't see anything that screams wrong at me, and it works so..
I'll defer for final approval to the Graph team :)

Thanks for tagging me for the learning experience, and congrats on shipping a customer request in <48h after we heard about it on a call!

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.

Thank you for the PR. I appreciate your enthusiasm to get your hands dirty and help out.

I noticed that there are lots of test cases missing. During the first pass, we should be aiming for coverage of as many of the various syntax features as possible, so that we don't have to keep coming back to fix small things (it's also quite likely that if some things work and other don't, users won't like the UX but they won't dislike it enough to complain about it).

I've outlined some in the inline comments, some more that are missing (non-exhaustive):

  • Type annotations (simple identifiers as well as generic types)
  • Escaped identifiers such as `class`.
  • Chained amends expression
  • Field accesses
  • Class definitions
  • Method definitions
  • Method calls
  • new expressions
  • Various property keywords such as fixed, hidden, local, const etc.
  • Annotations such as @Deprecated

You can go through the language reference (https://pkl-lang.org/main/current/language-reference/index.html) and add test cases for these.

If it's not possible to pick out some syntax features based on the grammar, we should still add a test case for that and note in a comment that it is because of an upstream limitation.

I do not feel comfortable merging this in its current state given the low coverage. Please re-request code review once you've made the suggested changes.

@mmanela
Copy link
Contributor Author

mmanela commented Mar 29, 2024

@varungandhi-src Thanks for the thorough review ( I was eager for you to show me the ropes here ).

Couple questions /comments

  1. By add unit tests in this case you mean make my pkl test file cover all the key language features you layout right?

  2. The scm file I copied from the tree sitter pkl repo, I just assumed it was correct so thanks for poking at that. Good opportunity to understand more there and make sure that file is correct.

  3. I will make a second pass today and make as many changes as I can.

@varungandhi-src
Copy link
Contributor

varungandhi-src commented Mar 29, 2024

By add unit tests in this case you mean make my pkl test file cover all the key language features you layout right?

I skimmed through the language reference (https://pkl-lang.org/main/current/language-reference/index.html) and noted things which looked missing. I did not look at everything in the language reference. When you're adding test cases, you should add test cases for the different syntactic features as written in the language reference (my list is a subset).

Generally, it is also good to add test cases for "interesting" combinations of features, e.g. how I wrote "Multiline strings, with and without interpolation, with and without leading whitespace." What exactly is an "interesting" combination is hard to define, but it's useful to think through variations. You may also want to look at the grammar.js file directly for inspiration, or the test cases in the tree-sitter-pkl repo. (https://github.com/apple/tree-sitter-pkl/tree/main/corpus)

The scm file I copied from the tree sitter pkl repo, I just assumed it was correct so thanks for poking at that. Good opportunity to understand more there and make sure that file is correct.

  1. Please add a link and attribution at the top of the file as the original is Apache 2.0 licensed.

  2. That's why we have the snapshot tests; you see there is this line https://github.com/apple/tree-sitter-pkl/blob/main/queries/highlights.scm#L101 but in your snapshot output, amends was getting marked as an identifier. I think this might be because the order matters -- generally the identifier queries should come at the end, and keywords and operators should come at the beginning (take a look at the highlighting queries for other languages as examples). So it seems like the upstream queries are buggy (and if you look at the language reference, it looks like the code snippets also sometimes have highlighting which looks a bit odd).

    If you're feeling so inclined, you could submit an issue or PR upstream too and point them to your improved queries.

@mmanela
Copy link
Contributor Author

mmanela commented Mar 29, 2024

@varungandhi-src Thanks for your detailed information. I made a second attempt to address as much of your feedback as I could figure out.

@mmanela mmanela requested a review from varungandhi-src March 29, 2024 19:24
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.

Looking much better, will do a further pass later.

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.

There are still a couple of small things which needs addressing, but approving so you can merge once those are fixed.

mmanela and others added 6 commits April 2, 2024 09:19
Co-authored-by: Varun Gandhi <varun.gandhi@sourcegraph.com>
…s/pkl/highlights.scm

Co-authored-by: Varun Gandhi <varun.gandhi@sourcegraph.com>
…s/pkl/highlights.scm

Co-authored-by: Varun Gandhi <varun.gandhi@sourcegraph.com>
…s/pkl/highlights.scm

Co-authored-by: Varun Gandhi <varun.gandhi@sourcegraph.com>
@mmanela mmanela merged commit 665c147 into main Apr 4, 2024
12 checks passed
@mmanela mmanela deleted the pkl_highlighting branch April 4, 2024 16:28
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.

3 participants