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

Gem sources #11

Merged
merged 8 commits into from
Oct 6, 2015
Merged

Gem sources #11

merged 8 commits into from
Oct 6, 2015

Conversation

smellsblue
Copy link
Contributor

Factor gem sources to correspond to upstreams, use Rack middleware to figure out which source to use, and push all meaningful logic out of Gemstash::Web.

@pcarranza
Copy link
Contributor

Hold on on this change. I think there are a few things missing. Let me review better when I get home.

@smellsblue
Copy link
Contributor Author

@pcarranza thanks for the review! And no rush, I'm not able to work on gemstash till later tonight probably anyways :-)

@smellsblue
Copy link
Contributor Author

I was also thinking of adding an integration test against the redirect strategy later, I'll push to this branch when I get to it.

private

def web_helper
@web_helper ||= Gemstash::WebHelper.new(server_url: env["gemstash.upstream"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to share a thought, I think we got to the same place with 2 different paths.

The branch I am working with I am basically building the "helper" (which I renamed to HTTPClient), but I am injecting a client builder which knows how to return something that knows how to create a client. This is so basically because of the same thing: we are changing the upstream url.

By injecting this "builder", you can also inject it in testing, and then you don't need to stub the methods to control that in testing. But this is a WIP in my branch still.

@pcarranza
Copy link
Contributor

I like it, I actually like a lot to handle the path parsing in the middleware, but I think that we are spreading the logic of how to handle the upstream (by calling chomp_path from many places) too much.

I think that the concept of the upstream as a concern is there asking to be extracted to a professional object, and that the web_helper is being stretched by not creating an HTTPClient which is what we actually need.

That said, let's move forward because I think that it is a step in the right direction. Then I can rebase my changes on top of this and start injecting the abstractions I already have in my WIP branch.

@pcarranza pcarranza merged commit a5e7410 into rubygems:master Oct 6, 2015
@smellsblue smellsblue deleted the gem-source branch October 7, 2015 05:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants