fix: Fix confusing/buggy push_scope behavior with callbacks (getsentry#156)

untitaker · web-flow · commit 3fe8712842d0 · 2018-10-31T14:51:40.000+01:00
* fix: Fix confusing/buggy push_scope behavior

* fix: Styling

* fix: Add assertion instead of hopefully noop conditional
diff --git a/sentry_sdk/hub.py b/sentry_sdk/hub.py
@@ -86,8 +86,7 @@ def __init__(self, hub, layer):
 
     def __enter__(self):
         scope = self._layer[1]
-        if scope is None:
-            scope = Scope()
+        assert scope is not None
         return scope
 
     def __exit__(self, exc_type, exc_value, tb):
@@ -271,21 +270,24 @@ def push_scope(self, callback=None):
         that should be used to pop the scope again.  Alternatively a callback
         can be provided that is executed in the context of the scope.
         """
+        if callback is not None:
+            with self.push_scope() as scope:
+                callback(scope)
+            return
+
         client, scope = self._stack[-1]
         new_layer = (client, copy.copy(scope))
         self._stack.append(new_layer)
 
-        if callback is not None:
-            if client is not None:
-                callback(scope)
-        else:
-            return _ScopeManager(self, new_layer)
+        return _ScopeManager(self, new_layer)
+
+    scope = push_scope
 
     def pop_scope_unsafe(self):
         """Pops a scope layer from the stack. Try to use the context manager
         `push_scope()` instead."""
         rv = self._stack.pop()
-        assert self._stack
+        assert self._stack, "stack must have at least one layer"
         return rv
 
     def configure_scope(self, callback=None):
@@ -305,16 +307,6 @@ def inner():
 
         return inner()
 
-    def scope(self, callback=None):
-        """Pushes a new scope and yields it for configuration.
-
-        The scope is dropped at the end of the with statement.  Alternatively
-        a callback can be provided similar to `configure_scope`.
-        """
-        with self.push_scope():
-            client, scope = self._stack[-1]
-            return self.configure_scope(callback)
-
 
 GLOBAL_HUB = Hub()
 _local.set(GLOBAL_HUB)
diff --git a/tests/test_basics.py b/tests/test_basics.py
@@ -1,4 +1,7 @@
 import logging
+
+import pytest
+
 from sentry_sdk import (
     Client,
     push_scope,
@@ -144,6 +147,32 @@ def test_push_scope_null_client(sentry_init, capture_events):
     assert len(events) == 0
 
 
+@pytest.mark.parametrize("null_client", (True, False))
+def test_push_scope_callback(sentry_init, null_client, capture_events):
+    sentry_init()
+
+    if null_client:
+        Hub.current.bind_client(None)
+
+    outer_scope = Hub.current._stack[-1][1]
+
+    calls = []
+
+    @push_scope
+    def _(scope):
+        assert scope is Hub.current._stack[-1][1]
+        assert scope is not outer_scope
+        calls.append(1)
+
+    # push_scope always needs to execute the callback regardless of
+    # client state, because that actually runs usercode in it, not
+    # just scope config code
+    assert calls == [1]
+
+    # Assert scope gets popped correctly
+    assert Hub.current._stack[-1][1] is outer_scope
+
+
 def test_breadcrumbs(sentry_init, capture_events):
     sentry_init(max_breadcrumbs=10)
     events = capture_events()
