Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add support for formatting capabilities to the LSP #1216

Merged
merged 12 commits into from
Apr 13, 2023
Merged

Conversation

ebresafegaga
Copy link
Contributor

@ebresafegaga ebresafegaga commented Mar 31, 2023

This pull request adds the formatting capability to the LSP using topiary.

To use this successfully, the user has to do the following:

  1. Have the topiary binary installed. See here
  2. Have the topiary repo cloned locally
  3. Set the environment variable TOPIARY_REPO_DIR to point to the local copy of the repo
  4. And finally, an environment variable TOPIARY_LANGUAGE_DIR to point to $TOPIARY_REPO_DIR/languages

Steps 2-4 are necessary because, for now, topiary cannot be used outside its repo directory.

Alternatives

I think making a user fetch the topiary and set those environment variables, just to have the formatting capability in nls is a bit too much. I can think of the following alternatives, but I don't know if they are ideal.

Keep a cache of the topiary repo

Keep a cache of the topiary repo. Fetch the repo from GitHub if it is not available in the local cache or not up to date.

  • Pros
    • Automatic updates of the repo (and hence the formatting rules for Nickel)
  • Cons
    • We still have to set environment variables at runtime
    • nls has to download a potentially large repo

Embedded Nickel formatting rules as a string in the nls binary

Since topiary just needs a single nickel.scm to be able to format nickel files we could just point TOPIARY_LANGUAGE_DIR to <some-temp-directory>/language, and put the embedded file in that directory.

  • Pros:
    • It's just a single file, so it will be small
    • No need to download/fetch anything
  • Cons:
    • We still have to set environment variables at runtime
    • We have to ensure the formatting rules are up to date with topiary

@ebresafegaga ebresafegaga self-assigned this Mar 31, 2023
@github-actions github-actions bot temporarily deployed to pull request March 31, 2023 15:02 Inactive
@github-actions github-actions bot temporarily deployed to pull request March 31, 2023 15:42 Inactive
@ebresafegaga ebresafegaga marked this pull request as ready for review April 4, 2023 13:50
@ebresafegaga ebresafegaga requested review from yannham and vkleen April 4, 2023 13:50
@github-actions github-actions bot temporarily deployed to pull request April 4, 2023 13:51 Inactive
@github-actions github-actions bot temporarily deployed to pull request April 4, 2023 15:07 Inactive
Copy link
Member

@yannham yannham left a comment

Choose a reason for hiding this comment

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

The setup is indeed a bit clunky, but I imagine it's due to the setup of Topiary being clunky, right? There's not more to do for the Nickel LSP than what we already need to do to use Topiary locally, if I understand correctly? In that case, this is a reasonable first iteration step 👍

@aspiwack
Copy link
Member

aspiwack commented Apr 5, 2023

The setup is indeed a bit clunky, but I imagine it's due to the setup of Topiary being clunky, right?

If there are things that Topiary could do to help, please open an issue there 🙂 .

@ebresafegaga
Copy link
Contributor Author

ebresafegaga commented Apr 5, 2023

The setup is indeed a bit clunky, but I imagine it's due to the setup of Topiary being clunky, right?

Yeah, but it's not really a big issue. The problem is that topiary cannot work (automatically) outside topiary repo's directory: you'd have to set some environment variables for it to work

If there are things that Topiary could do to help, please open an issue there 🙂 .

I've told @nbacquey about this. They (the topiary team) are aware of it, but they have not yet decided how dynamic loading of languages should work, so I assume there should be an issue tracking it there.

@github-actions github-actions bot temporarily deployed to pull request April 5, 2023 14:50 Inactive
@nbacquey
Copy link
Member

nbacquey commented Apr 6, 2023

Relevant issues in Topiary are tweag/topiary#254 and tweag/topiary#415

@yannham
Copy link
Member

yannham commented Apr 6, 2023

@nbacquey this is only about getting a running topiary executable, right? How would the $TOPIARY_LANGUAGE_DIR variable would be handled in this case? Would the queries be statically embedded inside such a binary?

@nbacquey
Copy link
Member

nbacquey commented Apr 6, 2023

Embedding the queries in the distributed binary would be a possibility, yes. Also, we could support manual installation of languages, as @ebresafegaga suggested. Something like

$ topiary install <language>

$ topiary list languages

@yannham
Copy link
Member

yannham commented Apr 6, 2023

@ebresafegaga is there anything left to do on this PR? Otherwise, I think you can merge.

@github-actions github-actions bot temporarily deployed to pull request April 13, 2023 11:32 Inactive
@ebresafegaga ebresafegaga merged commit 526f95a into master Apr 13, 2023
@ebresafegaga ebresafegaga deleted the lsp/formatting branch April 13, 2023 13:00
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants