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

Update access to rack constant #1917

Merged
merged 1 commit into from
Oct 16, 2019
Merged

Update access to rack constant #1917

merged 1 commit into from
Oct 16, 2019

Conversation

NikolayRys
Copy link
Contributor

@NikolayRys NikolayRys commented Oct 13, 2019

A small fix to keep up with the new changes in rack: rack/rack@6746515#diff-7c1a24d5b2fe58a6f925c7cacc6c55e7L558-R558

The format of the object with the possible statuses has changed in a recent release, which breaks the rack-edge appraisal, so this PR fixes it.

@grape-bot
Copy link

grape-bot commented Oct 13, 2019

1 Message
📖 We really appreciate pull requests that demonstrate issues, even without a fix. That said, the next step is to try and fix the failing tests!

Generated by 🚫 danger

@@ -213,7 +213,13 @@ def to_xml
context 'no content responses' do
let(:no_content_response) { ->(status) { [status, {}, ['']] } }

Rack::Utils::STATUS_WITH_NO_ENTITY_BODY.each do |status|
STATUSES_WITHOUT_BODY = if Rack::RELEASE >= '2.1.0'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use Gem::Version.new to compare, otherwise we're going to be in trouble with version 11.

https://medium.com/little-programming-joys/how-to-compare-version-strings-in-ruby-ced0916a4b71

Copy link
Contributor Author

@NikolayRys NikolayRys Oct 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll fix it.
It's done in this way here: https://github.com/ruby-grape/grape/blob/master/spec/grape/integration/rack_spec.rb#L23 So I've assumed it's how you would want it here too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That link doesn't go anywhere interesting ;) If you see it done elsewhere, please also fix!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It did until you've merged my other PR :)
But I'll fix this today, thank you

In rack 2.1.0 the list of statuses has become a hash with codes as keys. More info: rack/rack@6746515
@NikolayRys
Copy link
Contributor Author

Updated as suggested.

@dblock dblock merged commit f8b52e3 into ruby-grape:master Oct 16, 2019
@NikolayRys NikolayRys deleted the test_fixes branch October 16, 2019 15:18
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.

None yet

3 participants