Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Feb 8, 2023
Merged

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

merged 1 commit into from
Feb 8, 2023

Conversation

deivid-rodriguez
Copy link
Contributor

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

@deivid-rodriguez deivid-rodriguez mentioned this pull request Jun 3, 2022
Closed
@dorner
Copy link

dorner commented Jun 3, 2022

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

@deivid-rodriguez
Copy link
Contributor Author

deivid-rodriguez commented Jun 5, 2022 •
edited
Loading

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
Copy link

dorner commented Jun 7, 2022

@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
Copy link
Contributor Author

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

thor/lib/thor/actions.rb

Lines 204 to 234 in ab3b5be

# Loads an external file and execute it in the instance binding.
#
# ==== Parameters
# path<String>:: The path to the file to execute. Can be a web address or
# a relative path from the source root.
#
# ==== Examples
#
# apply "http://gist.github.com/103208"
#
# apply "recipes/jquery.rb"
#
def apply(path, config = {})
verbose = config.fetch(:verbose, true)
is_uri = path =~ %r{^https?\://}
path = find_in_source_paths(path) unless is_uri
say_status :apply, path, verbose
shell.padding += 1 if verbose
contents = if is_uri
require "open-uri"
URI.open(path, "Accept" => "application/x-thor-template", &:read)
else
open(path, &:read)
end
instance_eval(contents, path)
shell.padding -= 1 if verbose
end
, 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 deivid-rodriguez force-pushed the support-uris branch 2 times, most recently from 3684eae to 59ad393 Compare June 17, 2022 08:17
@deivid-rodriguez
Copy link
Contributor Author

@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.

dorner
dorner reviewed Jun 17, 2022
View reviewed changes
Copy link

@dorner dorner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. @rafaelfranca ?

@deivid-rodriguez
Copy link
Contributor Author

deivid-rodriguez commented Jun 23, 2022 •
edited
Loading

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 ruby/ruby#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
Support thor install <uri> to install remote thor files
439a593 
The previous code suggested that this was supported, but it was not
really working as expected.
@deivid-rodriguez
Copy link
Contributor Author

Rebased!

@deivid-rodriguez
Copy link
Contributor Author

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 deivid-rodriguez mentioned this pull request Dec 13, 2022
Merged
@rafaelfranca rafaelfranca merged commit 0e40ffa into rails:main Feb 8, 2023
@deivid-rodriguez
Copy link
Contributor Author

Thanks so much @rafaelfranca!

@deivid-rodriguez deivid-rodriguez deleted the support-uris branch February 8, 2023 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dorner dorner dorner left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@deivid-rodriguez @dorner @nobu @rafaelfranca