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

added spec for expected headers when error raised #1723

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gencer
Copy link
Contributor

@gencer gencer commented Dec 21, 2017

When we set headers using before, after or just before error! raised, we expect them to transferred with error response together.

  • First spec will be success due to there is no error.
  • Second test will fail at the moment because when an error raised, all headers will be ignored. They shouldn't be.

See: #1719

@grape-bot
Copy link

grape-bot commented Dec 21, 2017

1 Warning
⚠️ Unless you’re refactoring existing code, please update CHANGELOG.md.
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!

Here's an example of a CHANGELOG.md entry:

* [#1723](https://github.com/ruby-grape/grape/pull/1723): Added spec for expected headers when error raised - [@gencer](https://github.com/gencer).

Generated by 🚫 danger

When we set headers using `before`, `after` or just before `error!` raised, we expect them to transferred with error response together.

* First spec will be `success` due to there is no error.
* Second test will fail at the moment because when an error raised, all headers will be ignored. They shouldn't be.

added spec for expected headers when error raised

When we set headers using `before`, `after` or just before `error!` raised, we expect them to transferred with error response together.

* First spec will be `success` due to there is no error.
* Second test will fail at the moment because when an error raised, all headers will be ignored. They shouldn't be.
@gencer
Copy link
Contributor Author

gencer commented Dec 21, 2017

I've amended and squashed due to incorrect file format according to grape's rubocop rules. I've fixed it by this merge.

@dblock
Copy link
Member

dblock commented Dec 21, 2017

You should break up the specs: are headers not added from before and after in the error! case? If so we should call this expected behavior and document it. Then we can discuss whether we want to change it.

@gencer
Copy link
Contributor Author

gencer commented Dec 23, 2017

I will break up the specs asap.

For the question; after, before or above error!. On three cases, headers are stripped. The specs you see will fail on every one of them.

I will back with more proper specs as soon as possible.

@dblock
Copy link
Member

dblock commented Dec 27, 2017

I think we shouldn't change this behavior to avoid regressions for all existing users. PR documentation that says that the headers will not be included on error and specs that expect the opposite behavior of the ones here.

Lets find a way to create the opposite behavior. The error! call takes headers today. Any other suggestions?

@gencer
Copy link
Contributor Author

gencer commented Dec 27, 2017

Let's say we did not changed this behavior. As you mentioned we have error! with header support.

But fact is, for specific reasons, this could be enough. But I need to show some headers to clients even if they got error!.

One thing that i can suggest is making error-specific before/after statements like;

before do
   # usual before...
end

after do
   # usual after...
end

on_error do
    # execute and assign headers here on every error
end
# or...
error do
    # execute and assign headers here on every error (same as on_error just shorter name)
end

@dblock
Copy link
Member

dblock commented Dec 27, 2017

I like error do that would be an alias and possibly supersede rescue_from.

@gencer
Copy link
Contributor Author

gencer commented Dec 28, 2017

Definitely. This will allow us to specify headers, actions and anything else on every error, rather then define them on each error! call.

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