Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

ondfisk
Copy link

@ondfisk ondfisk commented Nov 19, 2023

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.

@ondfisk ondfisk requested a review from a team as a code owner November 19, 2023 14:01
Copy link
Member

@samruddhikhandale samruddhikhandale left a 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"
Copy link
Member

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.

Copy link
Member

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

Copy link
Member

@bamurtaugh bamurtaugh Nov 27, 2023

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.

Copy link
Member

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

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @eljog. Since we now have a couple requests for this, and I don't think I've seen any concerns against it, I'm fine with merging this change. What do you think @eljog?

Copy link
Member

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?

Copy link
Member

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?

Copy link
Contributor

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.

Copy link
Contributor

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 👍

Copy link
Member

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"
Copy link
Member

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!

Copy link
Member

Choose a reason for hiding this comment

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

Agree!

Copy link

@rafaelfranca rafaelfranca left a 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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants