Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Extracted request into a method object. #283

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

steveklabnik commented Feb 22, 2012

This will open the door for much more change in the future.

Currently a work in progress. I want to break up a few more things, add move persistance of connections back outside of the Request object, and name it better. But it's a start.

❤️ ❤️ ❤️ @evanphx ❤️ ❤️ ❤️

(Originally pull #282, till I did some silly git shit.)

Contributor

steveklabnik commented Feb 22, 2012

Why is GitHub so silly? Uuuuuugh.

steveklabnik added some commits Feb 10, 2012

@steveklabnik steveklabnik Made get_proxy_from_env private.
We never use this publcially, so it shouldn't be part of the
public interface. It's sorta akward to leave in, but
there wasn't any other code it fit in with, and I wasn't sure
that it was worth adding a whole second class just for
this one method.
4d37fdc
@steveklabnik steveklabnik Extracted Gem::UriFormatter.
There were a bunch of utility methods inside of RemoteFetcher that
didn't really belong there, so I pulled them out into a little class.
6b9f0c0
@steveklabnik steveklabnik Extracted request into a method object.
This will open the door for much more change in the future.

More things could be pulled into this object, but I'm not
ready to fight with all the tests such things break yet.
Basically, #user_agent and the @proxy_uri variable are
only used within this method, but there are a bunch of
tests that hack at their internal state.
589ae51
@steveklabnik steveklabnik Extracted user_agent into Request class. 3760004
Contributor

steveklabnik commented Feb 22, 2012

Fixed! rock. Silly rebase... I screwed it up.

@ghost ghost assigned zenspider and drbrain Mar 9, 2012

Contributor

zenspider commented Mar 9, 2012

I was just going to try to merge/patch this in, but @drbrain said he needs to do some work on some HTTPS stuff first, so I'm assigning to him. Hopefully this'll go in immediately afterwards.

Contributor

steveklabnik commented Mar 9, 2012

Cool. I have some more work I'm in the middle of too, I just haven't gotten a minute to work on it in a bit, I had all four of my wisdom teeth removed last week. o_O

Contributor

steveklabnik commented Mar 9, 2012

With that said, I can always make those changes after, so do feel free to merge this whenever you want. I'm going to continue to try and improve this portion of the codebase regardless.

Is there a reason why initialize is not a hash options? You could avoid this nil parameters and "fetch" default values on initialize.

Great work! Left my suggestion on steveklabnik/rubygems@3760004
About "move persistance of connections back outside of the Request object", you could build a ConnectionPool, something like https://github.com/mperham/connection_pool from Mike Perham.

@drbrain drbrain added a commit that referenced this pull request Jun 14, 2013

@drbrain drbrain Update history for #283 99fb9a0
Owner

drbrain commented Jun 14, 2013

Applied as @cb6d2e2 @d645e17 @9aa1148 @a175e5a @d5bfe9a with modifications due to bit rot in the patch.

@drbrain drbrain closed this Jun 14, 2013

@drbrain drbrain added a commit that referenced this pull request Jun 14, 2013

@drbrain drbrain Move some Gem::Request tests to test_gem_request
While #283 moved features over to Gem::Request it did not move the
corresponding tests.  This follows up that work by moving over more of
the tests.
c6680d0

@drbrain drbrain added a commit that referenced this pull request Jun 25, 2013

@drbrain drbrain Restore -V following #283 0b1b844

@drbrain drbrain added a commit that referenced this pull request Jul 22, 2013

@drbrain drbrain Restore block behavior for RemoteFetcher#request
Before #283 was merged RemoteRequest#request would yield the request
object to a block for customization.  This behavior lacked tests so it
was accidentally removed.

Now the behavior has been restored and a test has been added.

Fixes #602
5355973
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment