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 default ruby configuration for visual studio code #1958

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

Conversation

jenenliu
Copy link

@jenenliu jenenliu commented Apr 22, 2024

Motivation

I am trying to fix this issue #1619. Add recommand default configurations for Visual Studio code.

Implementation

Add [ruby] configuration to ConfigurationDefault in vscode/package.json

Automated Tests

Not yet

Manual Tests

Not yet

@jenenliu jenenliu requested a review from a team as a code owner April 22, 2024 13:57
@jenenliu
Copy link
Author

Hi @vinistock , I am trying to fix issue #1619, could you please help to guide how can I test ? I try to run Ruby-LSP: restart, but seems new configuration is not working. formatOnSave is not working at least. Thanks a lot.

@jenenliu
Copy link
Author

I have signed the CLA!

},
"[ruby]": {
"editor.defaultFormatter": "Shopify.ruby-lsp",
"editor.formatOnSave": true,

Choose a reason for hiding this comment

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

Sorry, but it doesn't seem to me that this should be the default value. When saving, I want to save what I am seeing now, unless I have explicitly defined it that way. This can be problematic especially when Rubocop is very aggressive in its corrections.

Copy link
Author

Choose a reason for hiding this comment

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

@brunoprietog Yep, I've updated the code, thx.

Copy link
Contributor

@andyw8 andyw8 Apr 23, 2024

Choose a reason for hiding this comment

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

We do want this to be formatOnSave, as it is the current recommendation. We believe this is the best approach for the vast majority of developers. Saving only runs the autocorrects that RuboCop treats as safe, so it's very unlikely that this would result in incorrect code.

Copy link
Author

Choose a reason for hiding this comment

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

So may I know do we have final decision for this ?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we do want this value to be true. The idea of the recommended defaults is for a user of the Ruby LSP to get all features out of the box, even the ones that might be turned off by default by VS Code.

For example, formatOnSave and formatOnType are both turned off by default, which leads to many people not realizing that the Ruby LSP can auto-complete end tokens for you as you type without the need to add any other extensions.

And remember, this is just a default recommendation. You can very easily turn this off.

Choose a reason for hiding this comment

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

formatOnType I understand, but formatOnSave I still think is dangerous. When you do the save action you expect a result. If you have manually included that feature, you know that there is a chance that it will be formatted on save, which is understandable. But if you have never enabled this option, what is expected is simply to save, without doing anything. Entering this option by default means that. Most people will not know that it is enabled and could run into unpleasant surprises when saving without knowing that this option is enabled. It is very different when you activate the option manually, because you really know what is going to happen.

Choose a reason for hiding this comment

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

Personally, I had this feature enabled at first, but I disabled it, as it happened to me several times that when applying a replace on several files at once, more than one file was left empty, and it took me quite some time to find out the reason. It has never happened again with formatOnSave disabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

@brunoprietog if you can reproduce, then please create an issue for this so that we can address.

Choose a reason for hiding this comment

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

It wasn't a problem with Ruby LSP, it was a problem with VS Code in general that happened to me with several file types, not just Ruby.

@vinistock
Copy link
Member

could you please help to guide how can I test ?

@jenenliu yes, absolutely. Here are the steps to test this:

  1. Before starting, ensure you don't have any of the recommended settings already configured in your VS Code. We want to verify that you get the recommended defaults automatically. If you have those settings, comment them out temporarily
  2. Now, we need to run the development version of the extension, so that you can verify the changes. First, ensure node dependencies are installed: cd vscode && yarn install
  3. In the run and debug panel, select the Run extension task and click start/run. This will open a second VS Code window, where the development version of the extension is running. It's in that window that you will be able to verify the changes
  4. In that second window, open a Ruby project
  5. Now test to see if the changes are taking effect. Are you seeing auto-formatting? When you type code, is the indentation level 2? If you are seeing those, then the changes work as expected

@jenenliu
Copy link
Author

could you please help to guide how can I test ?

@jenenliu yes, absolutely. Here are the steps to test this:

  1. Before starting, ensure you don't have any of the recommended settings already configured in your VS Code. We want to verify that you get the recommended defaults automatically. If you have those settings, comment them out temporarily
  2. Now, we need to run the development version of the extension, so that you can verify the changes. First, ensure node dependencies are installed: cd vscode && yarn install
  3. In the run and debug panel, select the Run extension task and click start/run. This will open a second VS Code window, where the development version of the extension is running. It's in that window that you will be able to verify the changes
  4. In that second window, open a Ruby project
  5. Now test to see if the changes are taking effect. Are you seeing auto-formatting? When you type code, is the indentation level 2? If you are seeing those, then the changes work as expected

@vinistock I've tested, it works as expected, thanks a lot. Now if formatOnSave should be included, then I'll update the code, and please help to approve.

@jenenliu jenenliu force-pushed the vs-default-better-configuration-ruby branch from 6c0d79e to 557ead9 Compare April 24, 2024 11:43
Copy link
Contributor

This pull request is being marked as stale because there was no activity in the last 2 months

@github-actions github-actions bot added the Stale label Jun 24, 2024
@jenenliu
Copy link
Author

@aryan-soni could you please help to review ? thx

@github-actions github-actions bot removed the Stale label Jun 25, 2024
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.

None yet

4 participants