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

Get modern rubyfmt working #628

Merged
merged 2 commits into from
Jun 16, 2020
Merged

Get modern rubyfmt working #628

merged 2 commits into from
Jun 16, 2020

Conversation

wadetandy
Copy link
Contributor

@wadetandy wadetandy commented Jun 7, 2020

Per #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
.
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
submitted in a PR
which will make it easier to install an actual executable shim script.

  • The build passes
  • TSLint is mostly happy
  • Prettier has been run

yarn lint is failing on a few things, but nothing in the changed files from this PR.

@codecov
Copy link

codecov bot commented Jun 7, 2020

Codecov Report

Merging #628 into master will increase coverage by 0.04%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #628      +/-   ##
=========================================
+ Coverage    5.04%   5.08%   +0.04%     
=========================================
  Files          10      10              
  Lines         119     118       -1     
  Branches       20      20              
=========================================
  Hits            6       6              
+ Misses        113     112       -1     
Flag Coverage Δ
#language_server_ruby 5.08% <0.00%> (+0.04%) ⬆️
Impacted Files Coverage Δ
...ges/language-server-ruby/src/formatters/RubyFMT.ts 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 19b8b34...bf4564b. Read the comment docs.

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.
@wingrunr21
Copy link
Collaborator

Hey, thanks for this. Looks good overall. I do prefer using a shim on the Rubyfmt side but if Penelope prefers to direct people to use an alias we could keep the direct ruby call. It looks like the invocation is pretty close to the old version just with the addition of the Rust native extension.

The inability to use an alias is linked to node's spawn. A bash alias requires an interactive shell in order to be expanded and spawn does not support using an interactive shell.

Previously this was passing in the file path. This caused rubyfmt to
ignore the current contents of the file on stdin, and instead just
formated the currently saved version of the file. A recent update to
rubyfmt will properly handle the input from stdin instead
@wingrunr21
Copy link
Collaborator

@wadetandy if this is ready I'm happy to merge it

@wadetandy
Copy link
Contributor Author

I think this is ready to go with the caveat that it won't really work until the corresponding shim script PR in rubyfmt gets merged, which should hopefully be this week sometime.

@wadetandy
Copy link
Contributor Author

Rubyfmt changes & vscode-ruby usage instructions were merged, so this should be GTG!

@wingrunr21 wingrunr21 merged commit 9ca9719 into rubyide:master Jun 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants