Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Roda Integration #2368
Roda Integration #2368
Changes from 20 commits
de8db34
3d03eda
910d7fe
9114586
403831a
c8953fd
352d1cb
68d3b5d
72553ad
ab7d7fe
6103e1f
fef14ff
76c4a38
586fd57
5016c7b
4c74c4b
f8c0c32
1462d1e
aa1db97
d16dbd5
a9db21c
2342811
41be369
585b862
affbc7a
4f107a0
3061065
fe099ac
af40cb6
c3804ba
edb4f2b
4555d3d
0aff08c
901297b
8510779
426f6cf
78237cc
c66542d
b3081e9
9323c6c
e18b260
034eef6
c3a1e2c
5aa0e94
29a9aa8
0b0fe25
3682891
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can move the error handling logic to the caller
So you could be confident yielding in the block without nested begin/rescue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like in the Roda code, dispatch can land on one of these methods (seen in this conditional)
Would I need to duplicate the rescue block and handle the error in both since either one of these can handle the dispatch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an update to this, I wasn't able to move the error handling logic anywhere else because it interfered with the errors produced by the underlying app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly strange edge case here where
request.request_method.to_s.upcase
raised an error, hit theensure
block, request raised an error, caught by thisrescue
block, thenrequest_method
will likely be undefined, possibly raising another error.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how this error can be thrown because all routes should be defined with a type from the start. Do you have a suggestion for handling this scenario?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I paired with Wan and it's not possible for
request.request_method.to_s.upcase
to fail, as it comes straight from Rack and it's part of the Rack contract. This is the same behaviour of our other Rack-ralated integrations (Sinatra, Rails, Hanami, Grace)