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 instructions for setting up vscode integration #201

Closed
wants to merge 3 commits into from

Conversation

wadetandy
Copy link
Contributor

No description provided.

@wadetandy
Copy link
Contributor Author

Fixes #125

@fables-tales
Copy link
Owner

Hi @wadetandy I think unfortunately this doesn't work! If you look at https://github.com/rubyide/vscode-ruby/blob/master/packages/language-server-ruby/src/formatters/RubyFMT.ts#L4 they vendor rubyfmt.rb (the legacy, broken) version of Rubyfmt in to their plugin. We should ask them either to update to point at the rust version or build our own plugin

@wadetandy
Copy link
Contributor Author

ahh interesting, didn't realize. I had run it against a few files in my project, but I guess the formatting issues were simple enough that this older bundled version worked fine. Opened an issue with the extension, so we can hopefully get the discrepancy resolved.

wadetandy added a commit to wadetandy/vscode-ruby that referenced this pull request Jun 7, 2020
Per rubyide#627, the previous behavior of the rubyfmt formatter class included
an embedded version of the rubyfmt script. This isn't really a workable
solution anymore as the program has evolved beyond a simple ruby script
to include native modules.  This instead changes the formatter class to
rely on a `rubyfmt` executable on the user's `$PATH`.  Because rubyfmt
is not currently installable via bundler and is in a pre-release state,
users are expected to have followed the necessary installation
instructions from the [project
README](https://github.com/penelopezone/rubyfmt/blob/master/README.md#how-do-i-use-it).
The current instructions recommend adding an alias, but vscode will not
respect that (or at least vscode-ruby doesn't). Instead I have a
[commit](fables-tales/rubyfmt@19d5946)
submitted in [a PR](fables-tales/rubyfmt#201)
which will make it easier to install an actual executable shim script.
@wadetandy
Copy link
Contributor Author

wadetandy commented Jun 7, 2020

@penelopezone Have opened a PR for vscode-ruby which the formatter to rely on a rubyfmt available on PATH. As that doesn't work correctly with an alias, I've also updated this PR to include a script for generating a shim executable in place of a bash alias. Glad to update it and/or the README however you'd like.

wadetandy added a commit to wadetandy/vscode-ruby that referenced this pull request Jun 7, 2020
Per rubyide#627, the previous behavior of the rubyfmt formatter class included
an embedded version of the rubyfmt script. This isn't really a workable
solution anymore as the program has evolved beyond a simple ruby script
to include native modules.  This instead changes the formatter class to
rely on a `rubyfmt` executable on the user's `$PATH`.  Because rubyfmt
is not currently installable via bundler and is in a pre-release state,
users are expected to have followed the necessary installation
instructions from the [project
README](https://github.com/penelopezone/rubyfmt/blob/master/README.md#how-do-i-use-it).
The current instructions recommend adding an alias, but vscode will not
respect that (or at least vscode-ruby doesn't). Instead I have a
[commit](fables-tales/rubyfmt@19d5946)
submitted in [a PR](fables-tales/rubyfmt#201)
which will make it easier to install an actual executable shim script.
The README currently recommends setting up a `rubyfmt` alias as part of
the installation process, but depending on how you are invoking the
command, not every environment will work with an alias.  This script
will install a shim script at the location of the user's choice:

``` bash
$ ./script/install_shim.sh ~/bin/rubyfmt
```

The script adds the `RUBYFMT_USE_RELEASE` variable as well so as to
prevent unnecessary debug output.
Originally wrote this test when trying to add the feature, but when I
pulled the latest changes, the CLI had already added the functionality,
so I figured I'd contribute the test back to it at least.
@wadetandy
Copy link
Contributor Author

Updated with a test for stdin processing, since I had separately started adding this for necessity with vscode integration and realized you'd already done it. Since I'd already written the test I figured I'd include it.

@wadetandy
Copy link
Contributor Author

Hey @penelopezone. Any feedback around why this was closed? Would love to figure out a path forward on supporting vscode and being able to leverage the existing plugin seems like the quickest path forward. Glad to rework this however you'd like, however.

@fables-tales
Copy link
Owner

OH! This was closed because I deleted the master branch and rebased all the commits to remove my deadname. You should retarget main and be able to re-open it! I didn't mean to close this :) Sorry for the churn! This looks great, full review in a bit.

@wadetandy
Copy link
Contributor Author

Ahh makes sense. Reopened #204 to address.

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.

2 participants