request.rb cleanup #124

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@calavera

Use String#scan and Hash#invert to generate the links hash.
Organize the connection block better.

David Calavera request.rb cleanup
Use String#scan and Hash#invert to generate the links hash.
Organize the connection block better.
7d069de
@travisbot

This pull request fails (merged 7d069de into 8a60bbf).

@calavera

I feel like this change has exposed a bug in the previous code. It's reasonable to think that if you set per_page to 100 for autolinking you want to set it for the next requests too, but this didn't happen before.

Where does that per_page come from anyways? I don't see any reference to it outside of the block.

@catsby
Member
catsby commented Aug 14, 2012

The per_page comes from the configuration options, which we then call attr_accessor for each in configuration.rb.

Tests in client_spec.rb lead me to believe the auto traversal / per page is persisted between requests, but I don't yet see why your code change should cause them to fail. Still looking...

@catsby catsby commented on the diff Aug 14, 2012
lib/octokit/request.rb
@@ -41,17 +41,16 @@ def request(method, path, options, version, authenticate, raw, force_urlencoded)
response = connection(authenticate, raw, version, force_urlencoded).send(method) do |request|
case method
when :delete, :get
- if auto_traversal && per_page.nil?
- self.per_page = 100
- end
+ per_page = 100 if auto_traversal && per_page.nil?
@catsby
catsby Aug 14, 2012 octokit member

If you change this to self.per_page = 100 ... the tests pass again. Still a little confused why :/

@calavera
calavera Aug 14, 2012

Yeah they pass because self.per_page= is setting a local variable inside the method, instead of setting the local variable in the connection.

Tests pass with that change because it sets it every time.

On Monday, August 13, 2012 at 8:07 PM, Clint wrote:

In lib/octokit/request.rb:

@@ -41,17 +41,16 @@ def request(method, path, options, version, authenticate, raw, force_urlencoded) > response = connection(authenticate, raw, version, force_urlencoded).send(method) do |request| > case method > when :delete, :get > - if auto_traversal && per_page.nil? > - self.per_page = 100 > - end > + per_page = 100 if auto_traversal && per_page.nil?
If you change this to self.per_page = 100 ... the tests pass again. Still a little confused why :/


Reply to this email directly or view it on GitHub (https://github.com/pengwynn/octokit/pull/124/files#r1369149).

@catsby
catsby Aug 23, 2012 octokit member

The way I'm reading it is setting per_page instead of self.per_page is setting a local variable, not the configuration option from lib/octokit/configuration.rb. When we reference per_page here on 44 we're reaching out to the config option, which is set to 50 in spec/octokit/client_spec.rb. We then set a local variable per_page to 100, because auto_traversal is true and per_page (from the config) is not nil. The local variable gets pushed into the hash and we've now overridden the value we set when we fist configured the client.

The spec mentioned expects a per_page value of 50, but here we create a local variable with value 100, which gets past on to the next loop when doing auto traversal. When we use self.per_page, we continue to use (and set) the configuration option, so the per page is in fact persisted across all the subsequent requests when using auto traversal.

In short, we should be using the client's per_page via self.per_page, unless I'm missing something. Which I haven't ruled out ;)

@pengwynn
Member

File in question has been refactored in #163

@pengwynn pengwynn closed this Nov 28, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment