Skip to content

Conversation

arthurgeek
Copy link
Contributor

No description provided.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling ab3bd7e on arthurgeek:patch-2 into cfe555e on lotus:master.

@jodosha
Copy link
Member

jodosha commented Jun 30, 2014

@arthurgeek It should be the opposite here: handle_exception ArgumentError => 400 and keep 400 where you amended.

@runlevel5
Copy link

@arthurgeek it should be 400 instead of 404. 404 is for missing document, files on server whilst 400 is for bad request.

@arthurgeek
Copy link
Contributor Author

@JONESLEE85 @jodosha yeah, I know, but since in the code it was 404, I just updated the return, but should had updated the code.

Should I close this PR and open a new one?

@runlevel5
Copy link

@arthurgeek I lost you here, can you please explain?

@arthurgeek
Copy link
Contributor Author

@JONESLEE85 take a look at README, line 812 (don't known how to link to file number in markdown files, as github only show rendered files). It needs to be the same as the line 839, right? I just switched the two. I was asking if I should close the PR, but just went ahead and made the right change, rebased, and amended the commit.

@arthurgeek arthurgeek changed the title Fix: the right status code here is 404 Fix: the right status code here is 400 Jul 6, 2014
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling b68a081 on arthurgeek:patch-2 into b8f5c52 on lotus:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling b68a081 on arthurgeek:patch-2 into b8f5c52 on lotus:master.

@runlevel5
Copy link

Please do :)

@arthurgeek
Copy link
Contributor Author

@JONESLEE85 I already did.

Sorry for the confusion :)

jodosha added a commit that referenced this pull request Jul 7, 2014
Fix: the right status code here is `400`
@jodosha jodosha merged commit 0c94bb3 into hanami:master Jul 7, 2014
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.

4 participants