-
Notifications
You must be signed in to change notification settings - Fork 508
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
Add support for Ruby extension #860
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution, left some comments.
@@ -24,7 +24,7 @@ | |||
"vscode": { | |||
// Add the IDs of extensions you want installed when the container is created. | |||
"extensions": [ | |||
"rebornix.Ruby" | |||
"shopify.ruby-extensions-pack" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bamurtaugh @craiglpeters Any thoughts on adding this extension?
This is an extension pack that bundles together a set of extensions useful for Ruby development. It might include extensions for Ruby language support, code formatting, linting, debugging, and other tools that can help with Ruby development.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we decide to add it, then can we add this to the Ruby Feature instead?
https://github.com/devcontainers/features/blob/main/src/ruby/devcontainer-feature.json#L21-L27
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening this PR @ondfisk and for looping me in @samruddhikhandale!
I love the idea of ensuring folks have the right set of extensions, though it's certainly a trickly problem to tackle as everyone has their own set of preferences. Since I'm not sure we've seen other requests to add this extension pack, my first instinct is we leave this PR open for further community feedback.
@ondfisk it could also be super helpful to share it in the community Slack channel to see if other Ruby dev container users would generally want this change: https://aka.ms/dev-container-community.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a new issue requesting this:
#870
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The feature already got the extension updated.
https://github.com/devcontainers/features/blob/3ea4d6bbd7864bcf7b5a91fdeeb66e4f5a6f46c0/src/ruby/devcontainer-feature.json#L21-L27
So, can we remove the extension from the image in this PR?
@ondfisk also please verify - the feature is installing "shopify.ruby-lsp
extension, but your proposal is to add shopify.ruby-extensions-pack
I assume the pack is coming with additional extensions. Any preference over one or the other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joshaber in relation to your PR, what you do think about the above suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm less certain about shopify.ruby-extensions-pack
because I haven't used it, but we should definitely (at minimum) remove rebornix.Ruby
since it's deprecated and since it sounds like the feature is installing the replacement extension.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated #860 to just remove rebornix.Ruby
since that seems like an obvious first step, regardless of where we land on shopify.ruby-extensions-pack
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have got the asks for this PR addressed as mentioned at #870 (comment)
I think we can close this PR in favor of the changes in the image and the ruby feature combined
@@ -24,7 +24,7 @@ | |||
"vscode": { | |||
// Add the IDs of extensions you want installed when the container is created. | |||
"extensions": [ | |||
"rebornix.Ruby" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing this makes sense to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While shopify.ruby-extensions-pack can be used by the community, it was made to be used by shopify. What we want to install is just shopify.ruby-lsp
, but that is already done in the ruby feature present on this image, so do we need to do this again here?
This pull request adds support for the "shopify.ruby-extensions-pack" extension in the devcontainer.json file. This extension provides additional functionality for Ruby development in Visual Studio Code. By including this extension in the devcontainer.json file, it will be automatically installed when the container is created, making it easier for developers to work with Ruby code in the development environment. This change improves the development experience for Ruby developers using this repository.