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

Use proper .format suffix if you have one path, issue 965 #1001

Merged
merged 1 commit into from
Apr 27, 2015
Merged

Use proper .format suffix if you have one path, issue 965 #1001

merged 1 commit into from
Apr 27, 2015

Conversation

hodak
Copy link
Contributor

@hodak hodak commented Apr 26, 2015

Hi,

so I had an issue #965 After investigating it all, it seemed like it was very unexpected behavior:

format :json
get '/users' => { status: 200, content_type: 'application/json' }
get '/users.json' => { status: 404 }

introduced with PR #809

It turned out it was strictly related to the issue I was having. I think it's logical to allow calling endpoint with specified format.

I saw @dblock comments about changing this (1, 2) so I decided to do this.

I've also added specs with the specific issue I was having with deeply nesting concerns.

Best

@@ -18,6 +18,7 @@
* [#988](https://github.com/intridea/grape/pull/988): Fixed duplicate identical endpoints - [@u2](https://github.com/u2).
* [#936](https://github.com/intridea/grape/pull/936): Fixed default params processing for optional groups - [@dm1try](https://github.com/dm1try).
* [#942](https://github.com/intridea/grape/pull/942): Fixed forced presence for optional params when based on a reused entity that was also required in another context - [@croeck](https://github.com/croeck).
* [#1001](https://github.com/intridea/grape/pull/1001/files): Fixed calling endpoint with specified format with format in its path.
Copy link
Member

Choose a reason for hiding this comment

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

Needs - [@hodak](...) like all the lines above, please.

@dblock
Copy link
Member

dblock commented Apr 27, 2015

As we're undoing behavior introduced in a prior version this also needs an UPGRADING note, please.

@dblock
Copy link
Member

dblock commented Apr 27, 2015

Thanks for doing this @hodak, needs some minor updates, see above. Please squash your commits too.

Both should work:
format :json
* /users
* /users.json
@hodak
Copy link
Contributor Author

hodak commented Apr 27, 2015

Done & done, thanks! ;)

@dblock
Copy link
Member

dblock commented Apr 27, 2015

Merging. I will add some notes on the regression.

dblock added a commit that referenced this pull request Apr 27, 2015
…e-965

Use proper .format suffix if you have one path, issue 965
@dblock dblock merged commit d31918e into ruby-grape:master Apr 27, 2015
dblock added a commit that referenced this pull request Apr 27, 2015
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.

2 participants