Fix Ruby 3.0 failure from double splat operator #63
Conversation
justingaylor
changed the title
| @@ -161,10 +164,10 @@ | |||
| lines.each { |line| line.gsub!(/\d+\.\d+(?= seconds)/, "x") } | |||
|
|
|||
| expect(lines).to eq([ | |||
| "[\e[32maaaaaa\e[0m] Running \e[33;1mruby #{non_zero_exit}\e[0m\n", | |||
| "[\e[32maaaaaa\e[0m] Running \e[33;1mruby #{escaped_path}\e[0m\n", | |||
There was a problem hiding this comment.
Layout/IndentArray: Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.
| @@ -132,17 +134,18 @@ | |||
| lines.each { |line| line.gsub!(/\d+\.\d+(?= seconds)/, "x") } | |||
|
|
|||
| expect(lines).to eq([ | |||
| "[\e[32maaaaaa\e[0m] Running \e[33;1mruby #{non_zero_exit}\e[0m\n", | |||
| "[\e[32maaaaaa\e[0m] Running \e[33;1mruby #{escaped_path}\e[0m\n", | |||
There was a problem hiding this comment.
Layout/IndentArray: Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.
| @@ -109,16 +110,17 @@ | |||
| lines.each { |line| line.gsub!(/\d+\.\d+(?= seconds)/, "x") } | |||
|
|
|||
| expect(lines).to eq([ | |||
| "[\e[32maaaaaa\e[0m] Running \e[33;1mruby #{zero_exit}\e[0m\n", | |||
| "[\e[32maaaaaa\e[0m] Running \e[33;1mruby #{escaped_path}\e[0m\n", | |||
There was a problem hiding this comment.
Layout/IndentArray: Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.
| @@ -42,7 +43,7 @@ | |||
| lines = output.readlines | |||
| lines.last.gsub!(/\d+\.\d+/, "x") | |||
| expect(lines).to eq([ | |||
| "[\e[32m#{uuid}\e[0m] Running \e[33;1mruby #{phased_output}\e[0m\n", | |||
| "[\e[32m#{uuid}\e[0m] Running \e[33;1mruby #{escaped_path}\e[0m\n", | |||
There was a problem hiding this comment.
Layout/IndentArray: Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.
| @@ -157,7 +159,7 @@ | |||
| lines = output.readlines | |||
| lines.last.gsub!(/\d+\.\d+/, "x") | |||
| expect(lines).to eq([ | |||
| "[\e[32m#{uuid}\e[0m] Running \e[33;1mruby #{phased_output}\e[0m\n", | |||
| "[\e[32m#{uuid}\e[0m] Running \e[33;1mruby #{escaped_path}\e[0m\n", | |||
There was a problem hiding this comment.
Layout/IndentArray: Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.
|
|
||
| output.rewind | ||
| lines = output.readlines | ||
| lines.last.gsub!(/\d+\.\d+/, "x") | ||
| expect(lines).to eq([ | ||
| "[\e[32m#{uuid}\e[0m] Running \e[33;1mruby #{non_zero_exit}\e[0m\n", | ||
| "[\e[32m#{uuid}\e[0m] Running \e[33;1mruby #{escaped_path}\e[0m\n", |
There was a problem hiding this comment.
Layout/IndentArray: Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.
justingaylor
force-pushed
the
fixDoubleSplatRuby3
branch
from
6fd810a
to
64b712b
Compare
|
I can try to fix up those RuboCop violations, but I think that will retrigger the |
Due to no shell escaping, when the file path contains spaces, local
tests fail with:
Failure/Error:
expect {
cmd.run("ruby #{cli}", input: infinite_input, timeout: 0.1)
}.to raise_error(TTY::Command::TimeoutExceeded)
expected TTY::Command::TimeoutExceeded, got #<TTY::Command::ExitError: Running `ruby /Volumes/1TB Seagate/src/tty-command/spec/fixtures/infinite_input` failed with
exit status: 1
stdout: Nothing written
stderr: ruby: No such file or directory -- /Volumes/1TB (LoadError)
> with backtrace:
# ./lib/tty/command.rb:106:in `run'
# ./spec/unit/timeout_spec.rb:33:in `block (3 levels) in <top (required)>'
# ./spec/unit/timeout_spec.rb:32:in `block (2 levels) in <top (required)>'
# ./spec/unit/timeout_spec.rb:32:in `block (2 levels) in <top (required)>'
This fails in Ruby 3.0+
wrong number of arguments (given 0, expected 1)
When TTY::Command.initialize uses the double splat operator, Ruby 3+
will fail this new test with the following error:
expected no Exception, got #<ArgumentError: wrong number of arguments (given 1, expected 0)> with backtrace:
# ./lib/tty/command.rb:54:in `initialize'
# ./spec/unit/command_spec.rb:5:in `new'
# ./spec/unit/command_spec.rb:5:in `block (3 levels) in <top (required)>'
# ./spec/unit/command_spec.rb:5:in `block (2 levels) in <top (required)>'
# ./spec/unit/command_spec.rb:5:in `block (2 levels) in <top (required)>'
justingaylor
force-pushed
the
fixDoubleSplatRuby3
branch
from
dd46cd8
to
3ce82db
Compare
There was a problem hiding this comment.
Hi Justin 👋
Thank you for looking into this issue and trying to resolve it. 🙏
Unfortunately, I don't believe we should be reverting to options hash. This change was done a long time ago and anyone who has used this gem should've already adjusted to using keywords. I see options hash as an antipattern. I no longer design my gems this way and wish tty-command to follow. Going forward, my preference is to use explicit keyword names and have Ruby on my side. Therefore I don't believe anything to be broken and thus need fixing on Ruby 3.0 as far as the initialisation goes.
As for the tty gem, it is broken in many ways. It is also not the way I wish to encourage building command-line applications. It was an early attempt and I'm working on overhauling the experience. First, I'll need to get other dependencies ready. What I'm trying to say is that we shouldn't worry about the impact on the tty.
Lastly, great find with escaping the path. However, I think we should fix it in the helper method to improve maintenance. I'd accept a PR specifically fixing only that if you have time.
Describe the change
This PR fixes a Ruby 3.0 failure (warning in Ruby 2.7) when using double splat operator/keyword params similar to this fix:
5f422f8
When using Ruby 3+ on
tty, we'll encounter this error (intty-command) trying to make a new app:By fixing this issue in
tty-commandand bumping the version intty, hopefully we'll be a step closer to being able to usettywith Ruby 3+. (There is a separate"*': negative argument (ArgumentError)"issue that happens intty` after this is fixed, so I will look into that after this gem gets fixed up.)For further on the exact issue, read here.
Also, unit tests also were updated for a separate issue. When there is a space in the path to the checked out repo base, specs fail due to
TTY::Commandinvoking ruby scripts in numerous places without shell escaping. This is fixed so specs will run for everybody.Why are we doing this?
tty-commanddoesn't work on Ruby 3.0 or higher.Benefits
We can more easily move
tty-command(and thus the greaterttytoolkit) forward to Ruby 3! 🚀Drawbacks
None.
Requirements
masterbranch?