Skip to content
This repository has been archived by the owner on Jul 31, 2023. It is now read-only.

Support Rufo for formatting #295

Closed
wingrunr21 opened this issue Mar 13, 2018 · 12 comments
Closed

Support Rufo for formatting #295

wingrunr21 opened this issue Mar 13, 2018 · 12 comments
Labels
feature-request Adds currently unsupported functionality

Comments

@wingrunr21
Copy link
Collaborator

Add support for using rufo for formatting instead of Rubocop

@wingrunr21 wingrunr21 added the feature-request Adds currently unsupported functionality label Mar 13, 2018
@gurgeous
Copy link
Contributor

Per discussion in #285, there are two ways to implement this:

  1. Add support for ruby.format = rubocop|rufo|false.
  2. Add support for ruby.format = rubocop|false, and encourage people to use vscode-rufo for rufo support.

(In both cases ruby.format will default to rubocop if ruby.lint.rubocop is set)

Personally I'd opt for (2) since I think it's good practice to utilize other extensions and minimize our own codebase. But I don't have a strong opinion. How should I proceed?

@gurgeous
Copy link
Contributor

I'll start with (2) until someone tells me otherwise. I'll work on top of #297 since this is also related to options processing.

@gurgeous
Copy link
Contributor

Any preference between false, 'none', and 'other'? Maybe if we can agree here we can apply it to other parts of the config. I'm just asking because @castwide proposed 'other', we have some existing code that uses 'none', and in my ignorance I suggested false. :)

@castwide
Copy link
Contributor

I don't have any particular preference, other than standardizing around a common convention. false might be a good compromise, since it covers the meaning of both other (I get this feature from somewhere else) and none (I don't want this feature at all).

@gurgeous
Copy link
Contributor

Working on this now. I'll use false.

Another tidbit - I believe the current implementation ALWAYS attempts to format with rubocop (regardless of settings). That was acceptable since it failed so quietly. Now that we have loud error messages, I think we should make it so that you have to opt into formatting by setting ruby.format to rubocop. Existing users will need to make a config change to re-enable formatting. I don't think this is a huge deal since I bet that rubocop formatting is broken for most existing users due to the vscode timeout issue.

@castwide
Copy link
Contributor

While experimenting with configuration options for ruby.codeCompletion and ruby.intellisense, I ran into a couple of issues with creating a false option. The first thing I tried was adding false to the configuration's enum. If the configuration's type is string, setting it to false on the Settings page reports a type error. If you set the type to ["string", "boolean"], the Settings page adds true to the completion options, even though true is not in the enum.

A couple possible solutions:

  • Use the string "false" instead of the value false (if we do this, we might be able to find a better word to use)
  • Use an empty string or null

@gurgeous Any thoughts?

@castwide
Copy link
Contributor

castwide commented Mar 13, 2018

Well, this is interesting. Out of curiosity, I tried to set the configuration's type to ["string", "false"], and it worked.

Edit: Spoke too soon. Selecting false still gets marked as an error.

@gurgeous
Copy link
Contributor

In my PR I use this:

        "ruby.format": {
          "type": [ "boolean", "string" ],
          "enum": [ false, "rubocop" ],
          "default": false,
          "description": "Which system to use for formatting, or false for no formatting"
        }

and it correctly flags true as invalid:

image

Seems fine, I think. Does anyone disagree? I don't feel strongly about any of these options.

@castwide
Copy link
Contributor

I used a similar configuration for ruby.codeCompletion:

"ruby.codeCompletion": {
  "type": ["boolean", "string"],
  "enum": [false, "rcodetools"],
  "default": false,
  "description": "Method to use for code completion. Use `false` if another extension provides this feature."
}

It flags true the same way, which is fine. But it also adds true to the completion items:

image

@gurgeous
Copy link
Contributor

Huh. Completion presents it as an option. I hadn't noticed that. That little Edit pencil thing on the left seems to use the enum (doesn't show true), and the green squiggles are working properly...

@wingrunr21, what do you think? I really don't care either way.

@castwide
Copy link
Contributor

Microsoft tagged the completion item issue as a bug and added it to their backlog, so we should be okay to keep false as an option.

@gurgeous
Copy link
Contributor

Great! @wingrunr21 or @rebornix - can this be merged?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Adds currently unsupported functionality
Projects
None yet
Development

No branches or pull requests

3 participants