iangreenleaf / githubot

Github API access, tailored for Hubot
MIT License
107 stars 38 forks source link

Proposed: Return response from request #24

Open ryanjones opened 10 years ago

ryanjones commented 10 years ago

I need to gain access to the link header from my request that I've sent to github. I've done a quick fix for this by changing this chunk of code in Github.prototype.request:

  if ((200 <= (_ref2 = res.statusCode) && _ref2 < 300)) {
          return cb(responseData);
        } else {
          return _this._errorHandler({
            statusCode: res.statusCode,
            body: body,
            error: responseData.message
          });
        }

to this (passing back the response with the responseData):

  if ((200 <= (_ref2 = res.statusCode) && _ref2 < 300)) {
          return cb(responseData, res);
        } else {
          return _this._errorHandler({
            statusCode: res.statusCode,
            body: body,
            error: responseData.message
          });
        }

This allows me to do this (coffee):

    get_github_issues = (page) ->
      promise = new RSVP.Promise((resolve, reject) ->
        url = "#{url_api_base}/orgs/#{org_name}/issues?filter=all&page=#{page}&per_page=100&sort=created"
        github.get url, (issues, res) ->
          console.log res["headers"]["link"]
          resolve issues
        )
      promise

I don't think would cause any regression as it would be an optional parameter in the callback.

Let me know if this is something you'd considering merging and I can create a PR for it. On that note, if you have a better way to grab the headers I'm all ears!

iangreenleaf commented 10 years ago

Yeah, this seems like a legit need, and returning it as a second arg sounds good in this particular case.

My only concern is that the response data structure we return should be fairly generic - res["headers"]["link"] looks fine, I'm just wondering what the rest of it looks like. I don't want to return an object that's too closely tied to our HTTP backend - right now it's scoped-http-client, but I may drop in a different library in the future and I wouldn't want everyone's stuff to break from being tied to attributes specific to scoped-http-client.

But yeah, I'd definitely consider a pull request. It would be extra awesome if it came with a new test or two!