Refactor remote fetcher #3017
Merged
Refactor remote fetcher #3017
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
deivid-rodriguez
force-pushed
the
refactor_remote_fetcher
branch
from
ff9aad2
to
b1c9d7d
Compare
bronzdoc
approved these changes
Dec 6, 2019
|
@bundlerbot r+ |
ghost
pushed a commit
that referenced
this pull request
Dec 6, 2019
3017: Refactor remote fetcher r=bronzdoc a=deivid-rodriguez # Description: In rubygems/bundler#7460 I'm vendoring the `uri` library inside `bundler` to add support for using the `uri` version inside Gemfile's just like any other gem. However, that was not the only thing needed since `bundler/setup` also touches some code from this class in rubygems. So the initial approach I took was to duplicate all the code inside bundler, replacing all reference to `URI` with `Bundler::URI` (the vendored namespace). Like in rubygems/bundler@3bcc579. However, this is ugly and hard to maintain so in this PR I'm refactoring this class so that it is enough to override only one method in the `bundler` subclass, like this: https://github.com/bundler/bundler/blob/aa3a25d30383a16e002dd94c209a6078746204a7/lib/bundler/gem_remote_fetcher.rb # Tasks: - [x] Describe the problem / feature - [ ] Write tests - [x] Write code to solve the problem - [ ] Get code review from coworkers / friends I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md). Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
Build failed
|
4 tasks
ghost
pushed a commit
that referenced
this pull request
Dec 6, 2019
3020: Remove regular 2.3 testing r=hsbt a=deivid-rodriguez # Description: From the failures at #3017, it seems that `actions/setup-ruby` dropped support for testing against ruby 2.3. So, I'm removing 2.3.x from our matrix. To ensure support until we actually drop it, I replaced it with an entry in the `rvm` file. # Tasks: - [x] Describe the problem / feature - [ ] Write tests - [x] Write code to solve the problem - [ ] Get code review from coworkers / friends I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md). Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
ghost
pushed a commit
that referenced
this pull request
Dec 6, 2019
3020: Remove regular 2.3 testing r=hsbt a=deivid-rodriguez # Description: From the failures at #3017, it seems that `actions/setup-ruby` dropped support for testing against ruby 2.3. So, I'm removing 2.3.x from our matrix. To ensure support until we actually drop it, I replaced it with an entry in the `rvm` file. # Tasks: - [x] Describe the problem / feature - [ ] Write tests - [x] Write code to solve the problem - [ ] Get code review from coworkers / friends I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md). Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
|
I'm actually doing some more testing on the bundler side, and it seems that this refactoring is not actually necessary... 🤷♂️ |
deivid-rodriguez
force-pushed
the
refactor_remote_fetcher
branch
from
b1c9d7d
to
8d7db43
Compare
Expectation goes first.
`URI.parse` consistenly returns `InvalidURIError`, so we can remove one rescue exception class and make the parsing like it looks in other places of this file.
`URI.parse` never returns `nil`.
Because it's more straightforwards and avoid loading `URI`.
The command refers to the begin-rescue-end block, not to the URI::Generic condition.
And pass them. Since we already have URIs in most places, it's better to reuse them to converting them to strings and then reparsing them to build the same URI object.
deivid-rodriguez
force-pushed
the
refactor_remote_fetcher
branch
from
8d7db43
to
c669214
Compare
|
@bundlerbot merge |
ghost
pushed a commit
that referenced
this pull request
Dec 9, 2019
3017: Refactor remote fetcher r=hsbt a=deivid-rodriguez # Description: In rubygems/bundler#7460 I'm vendoring the `uri` library inside `bundler` to add support for using the `uri` version inside Gemfile's just like any other gem. However, that was not the only thing needed since `bundler/setup` also touches some code from this class in rubygems. So the initial approach I took was to duplicate all the code inside bundler, replacing all reference to `URI` with `Bundler::URI` (the vendored namespace). Like in rubygems/bundler@3bcc579. However, this is ugly and hard to maintain so in this PR I'm refactoring this class so that it is enough to override only one method in the `bundler` subclass, like this: https://github.com/bundler/bundler/blob/aa3a25d30383a16e002dd94c209a6078746204a7/lib/bundler/gem_remote_fetcher.rb # Tasks: - [x] Describe the problem / feature - [ ] Write tests - [x] Write code to solve the problem - [ ] Get code review from coworkers / friends I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md). Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
Build succeeded
|
ghost
merged commit
|
As I mentioned in the previous comment, this PR wasn't necessary in the end, but it doesn't hurt either I think, so thanks for reviewing and merging :) |
|
@hsbt please don't merge PR's tagged as |
|
Right, I marked it as WIP because I wanted to tweak something else before setting it for review. Let me update what I wanted to improve on a separate PR. |
|
Forget about it, I think it's fine as it is. |
|
ah sorry. I believe we start to use draft pull request instead of wip label or wip title. It's great to see wip status for everyone. |
hsbt
pushed a commit
that referenced
this pull request
Dec 13, 2019
3017: Refactor remote fetcher r=hsbt a=deivid-rodriguez # Description: In rubygems/bundler#7460 I'm vendoring the `uri` library inside `bundler` to add support for using the `uri` version inside Gemfile's just like any other gem. However, that was not the only thing needed since `bundler/setup` also touches some code from this class in rubygems. So the initial approach I took was to duplicate all the code inside bundler, replacing all reference to `URI` with `Bundler::URI` (the vendored namespace). Like in rubygems/bundler@3bcc579. However, this is ugly and hard to maintain so in this PR I'm refactoring this class so that it is enough to override only one method in the `bundler` subclass, like this: https://github.com/bundler/bundler/blob/aa3a25d30383a16e002dd94c209a6078746204a7/lib/bundler/gem_remote_fetcher.rb # Tasks: - [x] Describe the problem / feature - [ ] Write tests - [x] Write code to solve the problem - [ ] Get code review from coworkers / friends I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md). Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
In rubygems/bundler#7460 I'm vendoring the
urilibrary insidebundlerto add support for using theuriversion inside Gemfile's just like any other gem. However, that was not the only thing needed sincebundler/setupalso touches some code from this class in rubygems.So the initial approach I took was to duplicate all the code inside bundler, replacing all reference to
URIwithBundler::URI(the vendored namespace). Like in rubygems/bundler@3bcc579.However, this is ugly and hard to maintain so in this PR I'm refactoring this class so that it is enough to override only one method in the
bundlersubclass, like this: https://github.com/bundler/bundler/blob/aa3a25d30383a16e002dd94c209a6078746204a7/lib/bundler/gem_remote_fetcher.rbTasks:
I will abide by the code of conduct.