Skip to content
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.

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

Fix rescue all header versioning regression #1114

Merged

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.

@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
…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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants