rails / thor

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

Support `thor install <uri>` to install remote thor files #787

Closed deivid-rodriguez closed 1 year ago

deivid-rodriguez commented 2 years ago

The previous code suggested that this was supported, but it was not really working as expected.

dorner commented 2 years ago

Tests seem to be failing. Also, can you please describe what the problem was and how this fixes it?

deivid-rodriguez commented 2 years ago

Yes, test are failing in the main branch. #780 should be merged first, and then I can rebase this to get it green hopefully :)

Sorry for not describing this enough.

As the title says, this PR intends to fix running thor install <uri>. The thor CLI provides an install command which receives a Thorfile as an argument. This argument can be a local filename, a directory (in which case Thor looks for a main.thor file inside) or a remote URI. However, passing a remote URI does not work at the moment and Thor raises an error like the following:

$ thor install https://raw.githubusercontent.com/rails/thor/main/Thorfile
Error opening file 'https://raw.githubusercontent.com/rails/thor/main/Thorfile'

To fix the problem, I detect whether the given argument is an uri or not. If it's an uri, I use URI.open, otherwise I use open like it's done now.

Let me know if there's more I should clarify!

dorner commented 2 years ago

@deivid-rodriguez I think the issue has to do with the changing of the functionality of open in Ruby 3.0. We'd need to make sure that older Rubies are still supported. https://stackoverflow.com/a/66032553/5199431

deivid-rodriguez commented 2 years ago

You're right, it sounds like this is broken since Ruby 3.0. And my new approach only works for rubies where URI.open is public.

I actually based this solution on what's done at https://github.com/rails/thor/blob/ab3b5be455791f4efb79f0efb4f88cc6b59c8ccf/lib/thor/actions.rb#L204-L234, so it sounds like that's already broken like in this PR.

Anyways, I will amend this to support old rubies once the repo gets some activity and CI in the main branch is back to green 👍.

deivid-rodriguez commented 2 years ago

@dorner This should be green now. As noticed, there's some code in thor/lib/thor/actions.rb where open-uri usage is also broken. But I'd rather investigate that on a separate PR.

deivid-rodriguez commented 2 years ago

For what it's worth, my initial motivation to change this was to avoid using

class Thor::Runner < Thor #:nodoc:
   autoload :OpenURI, "open-uri"
   # ...
end

because that just got deprecated and will no longer work as it works today in the future. See https://github.com/ruby/ruby/pull/5949.

But then when I attempted to change the code to not use the above, I realized that the code was not actually working, so I went ahead and fixed that too.

deivid-rodriguez commented 2 years ago

Rebased!

deivid-rodriguez commented 2 years ago

By the way, I think this PR is maybe a bit complicated to review due to indentation changes, I recommend looking at the patch with w=1: https://github.com/rails/thor/pull/787/files?w=1.

deivid-rodriguez commented 1 year ago

Thanks so much @rafaelfranca!