rails / thor

Thor is a toolkit for building powerful command-line interfaces.
http://whatisthor.com/
MIT License
5.14k stars 553 forks source link

Incorrect help for file_collision method without block #818

Closed shuuuuun closed 1 year ago

shuuuuun commented 1 year ago

Hello.

When using the file_collision method without block, displaying help will show commands (diff,merge) that should not be supported.

The reason for this behavior is that the branching is done by block_given? in the file_collision method, but not in file_collision_help. diff,merge works only when block is passed. https://github.com/rails/thor/blob/376e141adb594f3146c57e98151b97a20c93c484/lib/thor/shell/basic.rb#L287 https://github.com/rails/thor/blob/376e141adb594f3146c57e98151b97a20c93c484/lib/thor/shell/basic.rb#L308-L315 https://github.com/rails/thor/blob/376e141adb594f3146c57e98151b97a20c93c484/lib/thor/shell/basic.rb#L387-L397

Shouldn't the behavior be branched in the file_collision_help method as well, so that unsupported commands (diff,merge) are not displayed?

I checked with thor v1.2.1 and current main branch.

A working example is below:

# thor.rb
require "thor"

class MyCLI < Thor
  desc "hello", "say hello"
  def hello
    file_path = "./foo.txt"
    if !File.exist?(file_path)
      File.write(file_path, "first write", mode: "w")
      return
    end

    content = "file_collision with no block"
    puts "=== #{content}"
    if file_collision(file_path)
      File.write(file_path, content, mode: "w")
    end

    content = "file_collision with block"
    puts "=== #{content}"
    if file_collision(file_path) { content }
      File.write(file_path, content, mode: "w")
    end
  end
end

MyCLI.start(ARGV)
$ ruby ./thor.rb hello
=== file_collision with no block
Overwrite ./foo.txt? (enter "h" for help) [Ynaqh] h
        Y - yes, overwrite
        n - no, do not overwrite
        a - all, overwrite this and all others
        q - quit, abort
        d - diff, show the differences between the old and the new
        h - help, show this help
        m - merge, run merge tool
Overwrite ./foo.txt? (enter "h" for help) [Ynaqh] y
=== file_collision with block
Overwrite ./foo.txt? (enter "h" for help) [Ynaqdhm] h
        Y - yes, overwrite
        n - no, do not overwrite
        a - all, overwrite this and all others
        q - quit, abort
        d - diff, show the differences between the old and the new
        h - help, show this help
        m - merge, run merge tool
Overwrite ./foo.txt? (enter "h" for help) [Ynaqdhm] d
- file_collision with no block
+ file_collision with block
Retrying...
Overwrite ./foo.txt? (enter "h" for help) [Ynaqdhm] y

Thank you.

p8 commented 1 year ago

Hi @shuuuuun , Thanks for opening this issue. I think you are correct. Do you want to create a PR?

shuuuuun commented 1 year ago

@p8 Thanks for your comment! I have created the PR. https://github.com/rails/thor/pull/858