Skip to content
This repository has been archived by the owner. It is now read-only.

Cleanup some unnecessary code #7473

Merged
3 commits merged into from Dec 8, 2019
Merged

Cleanup some unnecessary code #7473

3 commits merged into from Dec 8, 2019

Conversation

deivid-rodriguez
Copy link
Member

@deivid-rodriguez deivid-rodriguez commented Dec 6, 2019

What was the end-user problem that led to this PR?

The problem was #7460 is very big so I want to extract these changes to a separate PR, so that we're more aware of them.

What was your diagnosis of the problem?

My diagnosis was that this code can be removed. In particular, in the GemRemoteFetcher class there was the following comment

https://github.com/bundler/bundler/blob/25595896eb0f8dfd004d941093bf1d8f4a39aeeb/lib/bundler/gem_remote_fetcher.rb#L9-L10

After having a look, I think the comment would make sense if where it says "gemstash", it actually meant "bundler". That would make sense to me since this is about fetching remote specs, so I would assume it's the client side running it.

Assuming this is the correct interpretation, we can remove the code since our minimum supported rubygems version is 2.5.2, and this code was included in rubygems in 2.5.0.

What is your fix for the problem, implemented in this PR?

My fix is to remove the GemRemoteFetcher class, plus simplify other related code.

I think the comment is misleading and it referred to rubygems, not
gemstash, because this code is about the client side of things. If this
is correct, we can drop it because we don't support any rubygems version
under 2.5.2, and the fix was introduced in rubygems 2.5.
@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Dec 8, 2019

No feedback, so I'll do this. It should be ok, I think.

@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Dec 8, 2019

@bundlerbot merge

ghost pushed a commit that referenced this issue Dec 8, 2019
7473: Cleanup some unnecessary code r=deivid-rodriguez a=deivid-rodriguez

### What was the end-user problem that led to this PR?

The problem was #7460 is very big so I want to extract these changes to a separate PR, so that we're more aware of them.

### What was your diagnosis of the problem?

My diagnosis was that this code can be removed. In particular, in the `GemRemoteFetcher` class there was the following comment

https://github.com/bundler/bundler/blob/25595896eb0f8dfd004d941093bf1d8f4a39aeeb/lib/bundler/gem_remote_fetcher.rb#L9-L10

After having a look, I think the comment would make sense if where it says "gemstash", it actually meant "bundler". That would make sense to me since this is about fetching remote specs, so I would assume it's the client side running it.

Assuming this is the correct interpretation, we can remove the code since our minimum supported rubygems version is 2.5.2, and this code was included in rubygems in 2.5.0.

### What is your fix for the problem, implemented in this PR?

My fix is to remove the `GemRemoteFetcher` class, plus simplify other related code.


Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
@ghost
Copy link

ghost commented Dec 8, 2019

Build succeeded

@ghost ghost merged commit 98a9137 into master Dec 8, 2019
3 checks passed
@ghost ghost deleted the misc_cleanup branch Dec 8, 2019
@indirect
Copy link
Member

indirect commented Dec 8, 2019

late feedback, but I like this :)

@deivid-rodriguez deivid-rodriguez modified the milestones: 2.1.0.rc, 2.1.0 Dec 13, 2019
deivid-rodriguez pushed a commit that referenced this issue Dec 13, 2019
7473: Cleanup some unnecessary code r=deivid-rodriguez a=deivid-rodriguez

### What was the end-user problem that led to this PR?

The problem was #7460 is very big so I want to extract these changes to a separate PR, so that we're more aware of them.

### What was your diagnosis of the problem?

My diagnosis was that this code can be removed. In particular, in the `GemRemoteFetcher` class there was the following comment

https://github.com/bundler/bundler/blob/25595896eb0f8dfd004d941093bf1d8f4a39aeeb/lib/bundler/gem_remote_fetcher.rb#L9-L10

After having a look, I think the comment would make sense if where it says "gemstash", it actually meant "bundler". That would make sense to me since this is about fetching remote specs, so I would assume it's the client side running it.

Assuming this is the correct interpretation, we can remove the code since our minimum supported rubygems version is 2.5.2, and this code was included in rubygems in 2.5.0.

### What is your fix for the problem, implemented in this PR?

My fix is to remove the `GemRemoteFetcher` class, plus simplify other related code.

Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
(cherry picked from commit bada03d)
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants