Fix Falcon Integration#2346
Conversation
c78d918 to
41016c8
Compare
|
|
||
| class SentryFalconMiddlewareBase(ABC): | ||
| @abstractmethod | ||
| def process_request(self, req: Any, resp: Any, *args: Any, **kwargs: Any) -> None: |
There was a problem hiding this comment.
Is it okay that I used Python 3-style type hints here? This file should never get loaded in Python 2
There was a problem hiding this comment.
I think this should be fine 👀
|
Should we be adding unit tests for this change? Or is it simple enough for unit tests to be unnecessary? |
sentrivana
left a comment
sentrivana
left a comment
There was a problem hiding this comment.
Great job figuring out how to deal with the py2/3 compat issue! 🎖️
Approving as the fix itself looks good, but please don't merge yet -- I think we do need testcases.
Even though the change looks small we're actually adding a major feature, which is support for ASGI Falcon, so I think that warrants some testcases. We could create a dummy ASGI Falcon app and basically check that the tests that work for WSGI also work for the async app. I see that there's a falcon.testing module -- maybe there's a test asgi app that we could use?
|
Converting back to a draft to block merging until test cases are completed |
|
This PR has been superseded by #2359 |
2 participants
Fixes #1946