Extracting a new "GemInstaller" from installer.rb #4063
Conversation
|
👍🏻 I like |
|
@indirect r+? |
|
@homu r+ |
|
📌 Commit 868a5a8 has been approved by |
Extracting a new "GemInstaller" from installer.rb My goal was to reveal the main part of install_gem_from_spec. From my perspective the main part is: 1. Install 2. Generate stubs 3. Print message to the user (4. handle exceptions) While working on it I found that Bundler::Installer's run method and instance variables weren't used when installing gems with install_gem_from_spec. The installer was (and still is) only used to generate executable stubs. So I inserted the new GemInstaller in ParallelInstaller; passing the instantiated Bundler::Installer to the GemInstaller only to generate the executable stubs. Based on this I would continue by extracting two BinstubGenerator classes and removing GemInstaller's dependency on Bundler::Installer, possibly allowing for a more concise way to generate binstubs in other parts of bundler. I was a bit intimidated by the amount of installers and weary to add another one to this contested namespace. So I checked in with @indirect who approved of the general direction of the refactoring and suggested to rename the old GemInstaller to RubyGemsGemInstaller.
|
💔 Test failed - status |
|
@A5308Y can you fix the failing tests? At least one is RuboCop, I'm not sure about the rest. Thanks! |
|
Sorry for the delay. I had a bike accident and hurt my dominant hand. I didn't even look at github-things for quite a while now. It's a lot better now. I still have to type carefully though. I see what I can do today in the (European) evening. Regarding the failure: |
|
Just doing |
|
I think it was a big that we previously rescued Exception, since that would stop ctrl-c from working. Let's change to StandardError (which is the default, and so you can just |
perspective the main part is: 1. Install 2. Generate stubs 3. Print message to the user (4. handle exceptions) While working on it I found that Bundler::Installer's run method and instance variables weren't used when installing gems with install_gem_from_spec. The installer was (and still is) only used to generate executable stubs. So I inserted the new GemInstaller in ParallelInstaller; passing the instantiated Bundler::Installer to the GemInstaller only to generate the executable stubs. Based on this I would continue by extracting two BinstubGenerator classes and removing GemInstaller's dependency on Bundler::Installer, possibly allowing for a more concise way to generate binstubs in other parts of bundler. I was a bit intimidated by the amount of installers and weary to add another one to this contested namespace. So I checked in with @indirect who approved of the general direction of the refactoring and suggested to rename the old GemInstaller to RubyGemsGemInstaller.
|
@indirect @segiddins I fixed this on Tuesday and just realized that there might not have been a notification. So, this is it. |
|
🚢 |
|
Hmm. Long time no comment. Can I do something to help the process? |
|
@homu r+ |
|
📌 Commit f7ee5ae has been approved by |
Extracting a new "GemInstaller" from installer.rb My goal was to reveal the main part of install_gem_from_spec. From my perspective the main part is: 1. Install 2. Generate stubs 3. Print message to the user (4. handle exceptions) While working on it I found that Bundler::Installer's run method and instance variables weren't used when installing gems with install_gem_from_spec. The installer was (and still is) only used to generate executable stubs. So I inserted the new GemInstaller in ParallelInstaller; passing the instantiated Bundler::Installer to the GemInstaller only to generate the executable stubs. Based on this I would continue by extracting two BinstubGenerator classes and removing GemInstaller's dependency on Bundler::Installer, possibly allowing for a more concise way to generate binstubs in other parts of bundler. I was a bit intimidated by the amount of installers and weary to add another one to this contested namespace. So I checked in with @indirect who approved of the general direction of the refactoring and suggested to rename the old GemInstaller to RubyGemsGemInstaller.
|
Strangely, it seems that the problem persists. So the previous problem was not transient. The status for #4077 (9 days ago) was correctly reported by Travis as you can see in here, but starting with #4085 (6 days ago) the API shows nothing. So do the recent two PRs, including the current PR. Did you happen to change the configuration for Travis and/or GitHub, maybe around 7 days ago? |
|
@barosl that's really, really strange. The Travis build log shows the merge for #4085 being built, and even references the correct commit: https://travis-ci.org/bundler/bundler/builds/88536039. Maybe Travis has a bug and is not reporting back to Github correctly? I'll check with them. |
|
@homu retry |
Extracting a new "GemInstaller" from installer.rb My goal was to reveal the main part of install_gem_from_spec. From my perspective the main part is: 1. Install 2. Generate stubs 3. Print message to the user (4. handle exceptions) While working on it I found that Bundler::Installer's run method and instance variables weren't used when installing gems with install_gem_from_spec. The installer was (and still is) only used to generate executable stubs. So I inserted the new GemInstaller in ParallelInstaller; passing the instantiated Bundler::Installer to the GemInstaller only to generate the executable stubs. Based on this I would continue by extracting two BinstubGenerator classes and removing GemInstaller's dependency on Bundler::Installer, possibly allowing for a more concise way to generate binstubs in other parts of bundler. I was a bit intimidated by the amount of installers and weary to add another one to this contested namespace. So I checked in with @indirect who approved of the general direction of the refactoring and suggested to rename the old GemInstaller to RubyGemsGemInstaller.
|
💔 Test failed - status |
|
@homu retry |
Extracting a new "GemInstaller" from installer.rb My goal was to reveal the main part of install_gem_from_spec. From my perspective the main part is: 1. Install 2. Generate stubs 3. Print message to the user (4. handle exceptions) While working on it I found that Bundler::Installer's run method and instance variables weren't used when installing gems with install_gem_from_spec. The installer was (and still is) only used to generate executable stubs. So I inserted the new GemInstaller in ParallelInstaller; passing the instantiated Bundler::Installer to the GemInstaller only to generate the executable stubs. Based on this I would continue by extracting two BinstubGenerator classes and removing GemInstaller's dependency on Bundler::Installer, possibly allowing for a more concise way to generate binstubs in other parts of bundler. I was a bit intimidated by the amount of installers and weary to add another one to this contested namespace. So I checked in with @indirect who approved of the general direction of the refactoring and suggested to rename the old GemInstaller to RubyGemsGemInstaller.
|
☀️ Test successful - status |
My goal was to reveal the main part of install_gem_from_spec. From my
perspective the main part is:
(4. handle exceptions)
While working on it I found that Bundler::Installer's run method and
instance variables weren't used when installing gems with
install_gem_from_spec. The installer was (and still is) only used to
generate executable stubs.
So I inserted the new GemInstaller in ParallelInstaller; passing the
instantiated Bundler::Installer to the GemInstaller only to generate
the executable stubs.
Based on this I would continue by extracting two BinstubGenerator
classes and removing GemInstaller's dependency on Bundler::Installer,
possibly allowing for a more concise way to generate binstubs in other
parts of bundler.
I was a bit intimidated by the amount of installers and weary to add
another one to this contested namespace. So I checked in with
@indirect who approved of the general direction of the refactoring and
suggested to rename the old GemInstaller to RubyGemsGemInstaller.