search

rombert / ereviewboard

A mylyn-based Eclipse integration for Review Board
46 stars 31 forks source link

Enhanced validation of api entry point #100

Closed selsemore closed 12 years ago

selsemore commented 12 years ago

Verify that api endpoint exists by checking that status != 404 rather than by checking that status == 200. This way the check works even though we haven't logged in yet.

buildhive commented 12 years ago

Robert Munteanu » ereviewboard #2 SUCCESS This pull request looks good (what's this?)

rombert commented 12 years ago

Thanks for submitting this fix. I have two comments:

  1. One of the commits is a pure whitespace change, so please drop it.
  2. Instead of checking status != 404, can we check for the exact statuses we know to expect, for instance {200, 401} ? This will prevent - for instance - a 403 or 500 status from being interpreted as correct.
selsemore commented 12 years ago

Thanks, Robert. I've made the change that you suggested. I'm sorry, but I'm quite new to GitHub and Git both. This is the first time I've forked a repository or sent a pull request. Can you tell me how I go about dropping commits from a pull request (replacing them with my latest)?

-----Original Message----- From: Robert Munteanu [mailto:reply+i-4650190- eae4c6e6da05db0ff456a991ed51d77a7697aa16-1742317@reply.github.com] Sent: Sunday, May 20, 2012 4:16 AM To: Steve Elsemore Subject: Re: [ereviewboard] Enhanced validation of api entry point (#100)

Thanks for submitting this fix. I have two comments:

  1. One of the commits is a pure whitespace change, so please drop it.
  2. Instead of checking status != 404, can we check for the exact statuses we know to expect, for instance {200, 401} ? This will prevent - for instance - a 403 or 500 status from being interpreted as correct.

Reply to this email directly or view it on GitHub: https://github.com/rombert/ereviewboard/pull/100#issuecomment- 5808147

buildhive commented 12 years ago

Robert Munteanu » ereviewboard #3 SUCCESS This pull request looks good (what's this?)

rombert commented 12 years ago

I think that you can either amend and then force push to your master branch or push to a different branch and re-issue the pull request.

Thanks!