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.

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

Autoload shellwords when it's needed. #6734

Merged
merged 1 commit into from
Jun 13, 2023

Conversation

ioquatix
Copy link
Contributor

@ioquatix ioquatix commented Jun 9, 2023

Attempt to fix #6568.

Some code paths weren't loading shellwords and I experienced some errors because of this. So, adding a top level autoload avoids this problem.

@fxn is it safe to add an autoload like this? i.e. are we at risk of clobbering anything if another project decided to do the same?

@ioquatix ioquatix force-pushed the autoload-shellwords branch 3 times, most recently from b0c2ea8 to 69287ac Compare June 9, 2023 13:16
@ioquatix ioquatix force-pushed the autoload-shellwords branch from 69287ac to e916ccb Compare June 9, 2023 13:21
@fxn
Copy link
Contributor

fxn commented Jun 9, 2023

@ioquatix I believe it should be mostly safe, but let me elaborate a bit to give elements to decide.

Adding an autoload is in practice like defining the constant, except that you defer the file evaluation. So this would be as "safe" as issuing a require call that defines Shellwords normally, in this respect.

On paper, this would conflict with code bases that define their own Shellwords top-level constant and also load RubyGems. However, who could do that before without loading "shellwords" as a side-effect one way or another? I believe it is really, really unlikely, if at all possible.

@ioquatix
Copy link
Contributor Author

ioquatix commented Jun 9, 2023

Thanks so much for your great clarification and assurances.

@ioquatix
Copy link
Contributor Author

ioquatix commented Jun 9, 2023

@deivid-rodriguez @simi I think this is good to go.

@deivid-rodriguez
Copy link
Member

Thanks!

@deivid-rodriguez deivid-rodriguez merged commit 127a20d into rubygems:master Jun 13, 2023
@ioquatix ioquatix deleted the autoload-shellwords branch June 13, 2023 11:49
deivid-rodriguez added a commit that referenced this pull request Jun 27, 2023
Autoload shellwords when it's needed.

(cherry picked from commit 127a20d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

uninitialized constant Gem::Ext::Builder::Shellwords
3 participants