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

Report endpoint_call.grape which includes the whole stack #2021

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Kukunin
Copy link

@Kukunin Kukunin commented Mar 17, 2020

Fixes #2010

To keep the backward compatibility, I introduced a new event. The integration works fine in my project with this change.

@Kukunin
Copy link
Author

Kukunin commented Mar 17, 2020

Rubocop limit was increased previously too 28d34ee#diff-74423563ec7c74f4cb7f74689b49e7c1R47, and I believe to refactor it, a person should a better insight about grape than I have.

So I increased limit by 3 lines too

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Feels a little light on specs and the failures look legit, too. Try to write specs that highlight the difference between these two instrumentations (the existing and the new)?

#### endpoint_run.grape

The main execution of an endpoint, includes filters and rendering.
Similar to `endpoint_call.grape` but does not include middlewares, thus might be inaccurate.
Copy link
Member

Choose a reason for hiding this comment

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

I think "inaccurate" is incorrect. I think here we want to highlight what this does, as opposed to what it doesn't. Maybe "Execution of the endpoint, excluding filters, middlewares or execution inside rescue_from blocks." + explain what happens during an exception?

#### endpoint_run.grape

The main execution of an endpoint, includes filters and rendering.
Similar to `endpoint_call.grape` but does not include middlewares, thus might be inaccurate.
One more difference that it returns original exceptions, before `rescue_from` handlers.
Copy link
Member

Choose a reason for hiding this comment

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

Should be part of the text above. But this is unclear. What does "returns" mean here?

@Kukunin
Copy link
Author

Kukunin commented Mar 17, 2020

Thanks for the review. Will try to fix the issues later today or tomorrow

Version 1.6.2
@dblock
Copy link
Member

dblock commented Oct 5, 2022

@Kukunin bumping myself on top of your list!

@Kukunin
Copy link
Author

Kukunin commented Oct 5, 2022

hi @dblock. do you mean that it'd be good to finalize and get this PR merged? If yes, then it's something I can do

@dblock
Copy link
Member

dblock commented Oct 5, 2022

hi @dblock. do you mean that it'd be good to finalize and get this PR merged? If yes, then it's something I can do

Yes, please! Rebase & al. I know it's old but I think we still want it.

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.

endpoint_run.grape callback is called without middlewares
2 participants