Skip to content

Fix rescue all header versioning regression #1114

New issue

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

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Aug 27, 2015
Merged

Fix rescue all header versioning regression #1114

merged 1 commit into from
Aug 27, 2015
+108 −26

Conversation

suan
Copy link
Contributor

@suan suan commented Aug 20, 2015

A suggested fix for #968

There are two failing specs which show that unfortunately there is no way to rescue_from an invalid version, since it's possibly valid and needs to propogate. This was also the case before the regression was introduced.

@suan suan mentioned this pull request Aug 20, 2015
Closed
@dblock
Copy link
Member

dblock commented Aug 20, 2015

The code looks good, but I am confused to what the spec failures mean - are the specs wrong? If so they need to change.

Also please update CHANGELOG and squash the commits.

@dblock
Copy link
Member

dblock commented Aug 20, 2015

I am curious what @dm1try thinks about this.

Also, this definitely needs to be documented in both README and the behavior change in UPGRADING.

Finally, maybe we need to make this behavior configurable?

@suan
Copy link
Contributor Author

suan commented Aug 21, 2015

@dblock The spec failures show that with my changes it's (unfortunately) not possible to rescue any Grape::Exceptions::InvalidHeaderVersions - as we'll always need to pass those through to allow multi-versioned endpoints to work. (If there's some other way that you know of, that'd be more ideal)

@croeck
Copy link
Contributor

croeck commented Aug 22, 2015

The changes seem good to me. All failing tests are invalid and did not regard the scenario as defined in the related issue.

@dblock
Copy link
Member

dblock commented Aug 22, 2015

@suan remove the bad tests, squash the commits please, update README, UPGRADING and CHANGELOG.

@suan suan force-pushed the fix_rescue_all_header_versioning_regression branch from 6793b57 to 54fbbf3 Compare August 26, 2015 22:52
@suan suan force-pushed the fix_rescue_all_header_versioning_regression branch from 54fbbf3 to 2fa517c Compare August 26, 2015 22:55
@suan
Copy link
Contributor Author

suan commented Aug 26, 2015

@dblock I've made the suggested changes - let me know if there's anything you'd need, thanks!

@dblock
Copy link
Member

dblock commented Aug 27, 2015

This is good, merging.

dblock added a commit that referenced this pull request Aug 27, 2015
@dblock
Merge pull request #1114 from suan/fix_rescue_all_header_versioning_r…
255100e 
…egression

Fix rescue all header versioning regression
@dblock dblock merged commit 255100e into ruby-grape:master Aug 27, 2015
@suan
Copy link
Contributor Author

suan commented Aug 27, 2015

Awesome, appreciate it @dblock!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@suan @dblock @croeck