httpserver: propagate event handler exceptions to httpserver#64
httpserver: propagate event handler exceptions to httpserver#64csernazs merged 2 commits intocsernazs:masterfrom
Conversation
csernazs
left a comment
There was a problem hiding this comment.
LGTM :) Kudos for adding thorough testing.
One thing I want to ask: why do you create a separate list for handler errors? Is this because of backward compatibility to guarantee that exceptions raised from the request handlers will still be ignored in check_assertions?
I'm thinking about adding one single check() function which would call these two method sequentially, so if someone is using a custom error handler they would benefit from both world. What do you think, would this make sense?
This would still keep the API compatible (check_assertions would not change) and if we document that the range of errors checked can change in the future, new calls could be added there later. Naming would be check() or check_all() or something like this.
|
sure, maybe the separation is not needed. happy to consolidate the two functions. |
|
I see the benefit of the separation (this wasn't true when I first saw your code, I admit). It provides backward compatibility and also provides some kind of differentiation of the "some expected result is not okay" between "some unhandled and nasty thing happened". AFAIK that's the difference between failure and error in the terms of testing and it is good to be separated. |
Codecov Report
@@ Coverage Diff @@
## master #64 +/- ##
==========================================
+ Coverage 94.05% 94.33% +0.28%
==========================================
Files 3 3
Lines 437 459 +22
==========================================
+ Hits 411 433 +22
Misses 26 26
Continue to review full report at Codecov.
|
|
It seems that there's an unused import in your commit, could you fix that?
|
|
i fixed the import error (my bad) and also added the |
|
Thanks! :) Just one little final ask: could you please squash these commits into one? It would look nicer. Then I will merge it I promise. |
|
You could squash and merge the PR :-) |
This PR fixes #63
I went for option 1 as proposed in #63
The way it is implemented now should not break user code, as exceptions from handlers are collected and then re-raised.
I also added
__pycache__to the.gitignore