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.

Sign up for GitHub

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

Add support for Ruby extension #860

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/ruby/.devcontainer/devcontainer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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!

"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 •
edited
Loading

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

]
}
},
Expand Down