Skip to content

Add snippets #53

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

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

Conversation

andyw8
Copy link
Contributor

@andyw8 andyw8 commented Mar 29, 2025

This PR adds a set of snippets to the add-on (see https://zed.dev/docs/snippets)

They were added to Ruby LSP in Shopify/vscode-ruby-lsp#150. I believe they were derived from Textmate's Ruby bundle. (incorrect)

The vscode-ruby-lsp repo was later combined into the ruby-lsp repo, so they now live at https://github.com/Shopify/ruby-lsp/blob/main/vscode/snippets.json. The file I'm adding here is an exact copy that file.

I think there's a opportunity to improve the snippets, so consider this as a starting point.

I'm currently investigating why snippets are appearing twice: solved by zed-industries/zed#28940

Note: It's possible that snippet completion may be later moved into the LSP: Shopify/ruby-lsp#1874

TODO:

@cla-bot cla-bot bot added the cla-signed label Mar 29, 2025
@andyw8
Copy link
Contributor Author

andyw8 commented Apr 3, 2025

One thing to note, long descriptions are truncated:

Screenshot 2025-04-03 at 11 20 36 AM

Here's how it shows up in VS Code:

Screenshot 2025-04-03 at 11 22 03 AM

I've opened an issue: Snippet selection should use key as label

@andyw8
Copy link
Contributor Author

andyw8 commented Apr 15, 2025

Also opened an issue for the snippets being listed twice:

@redforks
Copy link

redforks commented Apr 17, 2025

@andyw8
Copy link
Contributor Author

andyw8 commented Apr 17, 2025

@redforks thank you! I'll try on the next release.

Edit: Verfied working in 0.182.11

@andyw8 andyw8 changed the title WIP: Add snippets Add snippets Apr 18, 2025
@andyw8 andyw8 marked this pull request as ready for review April 18, 2025 12:42
@andyw8
Copy link
Contributor Author

andyw8 commented Apr 18, 2025

FYI @vitallium

@joeldrapper
Copy link
Contributor

I love the idea of having a nice set of default Ruby snippets, except the indentation is messed up and some of the shortcuts seem confusing to me. For example def here is a snippet for a singleton method, which seems odd.

@andyw8 andyw8 force-pushed the andyw8/add-snippets branch from 5f9b72c to 19715ac Compare April 22, 2025 19:46
@andyw8
Copy link
Contributor Author

andyw8 commented Apr 22, 2025

In Shopify/vscode-ruby-lsp#150 there were snippets for both def and def self. but for some reason the current version only has def self. I've added it back in. I've also renamed the singleton one's prefix to defs (as it was in the 2006 original).

I think the indentation is the same issue as reported in zed-industries/zed#15846

@joeldrapper
Copy link
Contributor

Looks like it. defs is nice! I’ve run into indentation issues with snippets before and couldn’t figure it out. Looks like this is why. It seems like the snippet templates were designed to assume the editor could always figure out the indentation by itself but I don’t know if that can always be the case. It would be useful if the template could store where to outdent and indent and then have it apply whatever your indentation characters are set to.

Copy link

@vinistock vinistock left a comment

Choose a reason for hiding this comment

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

Ideally, we would complete the work to move these to the Ruby LSP server to make it available for any editor without needing to copy the snippets around (in addition to making them smarter).

That said, if Zed wants a short term solution, this should do the trick.

@vitallium
Copy link
Collaborator

Thanks for adding the snippets! Before merging this, I'd like to check with the Zed team first to make sure they’re okay with modifying the license file.

@osiewicz Hi, sorry for pinging you directly, but could you please check if modifying the license file to include copyrights for other components is OK from the Zed team's perspective? Thanks!

@joeldrapper
Copy link
Contributor

@vitallium can I also suggest we wait on zed-industries/zed#15846 before merging this? The snippets look great, but I tried using them in a dev build locally and found I wanted to disable them almost immediately due to indentation issues.

@vitallium
Copy link
Collaborator

@vitallium can I also suggest we wait on zed-industries/zed#15846 before merging this? The snippets look great, but I tried using them in a dev build locally and found I wanted to disable them almost immediately due to indentation issues.

Yes, I played around with this PR a bit, and I also noticed some oddities with spacing and indentation.

@osiewicz
Copy link
Contributor

@vitallium Yep, these license changes look good to me (not a lawyer though).

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.

6 participants