Fix API regression #1729
Merged
Fix API regression #1729
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We incorrectly applied error handlers at each sub-route instead of applying it after grouping all sub-routes together. The result was that only `getinfo` could actually be called.
pm47
approved these changes
Mar 12, 2021
|
So routing was canceled after the first miss? Really sorry just tested the getinfo route with this commit as I was hunting for the json error formatting. Need to get this E2E test setup running to prevent my custom poor manual testing and be more careful next time. |
|
No worries, my tests should have caught that and failed to do so as well! |
tompro
pushed a commit
to tompro/eclair
that referenced
this pull request
Mar 21, 2021
We incorrectly applied error handlers at each sub-route instead of applying it after grouping all sub-routes together. The result was that only `getinfo` could actually be called.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
We incorrectly applied error handlers at each sub-route instead of applying it after grouping all sub-routes together. The result was that only
getinfo(the first one tried) could actually be called.The tests were changed to directly call the sub-routes so they didn't notice this issue. I fixed that by adding tests that failed with the previous code by simply using the top-level route.
I thought I had tested this E2E before merging #1690 but my (custom, poorly written) scripts in fact didn't correctly re-source my
.bash_aliasesso I was in fact testingmaster🤦cc @tompro