-
Notifications
You must be signed in to change notification settings - Fork 50
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
Conversation
Fixes #125 |
Hi @wadetandy I think unfortunately this doesn't work! If you look at |
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. |
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.
@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. |
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.
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. |
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. |
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. |
Ahh makes sense. Reopened #204 to address. |
No description provided.