From d241ccb9094eea28ca1b0b99c03dc8606d5a3e99 Mon Sep 17 00:00:00 2001 From: Winston Chang Date: Fri, 8 Dec 2023 22:30:27 -0600 Subject: [PATCH 1/2] Better messages for AttributeError in Shiny Express --- shiny/express/_run.py | 93 ++++++++++++++++++++++++------------------- 1 file changed, 53 insertions(+), 40 deletions(-) diff --git a/shiny/express/_run.py b/shiny/express/_run.py index 5626820e1..d8cb923fe 100644 --- a/shiny/express/_run.py +++ b/shiny/express/_run.py @@ -72,51 +72,64 @@ def set_result(x: object): nonlocal ui_result ui_result = cast(Tag, x) + prev_displayhook = sys.displayhook sys.displayhook = set_result - reset_top_level_recall_context_manager() - get_top_level_recall_context_manager().__enter__() - - file_path = str(file.resolve()) + try: + reset_top_level_recall_context_manager() + get_top_level_recall_context_manager().__enter__() + + file_path = str(file.resolve()) + + var_context: dict[str, object] = { + "__file__": file_path, + display_decorator_func_name: _display_decorator_function_def, + } + + # Execute each top-level node in the AST + for node in tree.body: + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): + exec( + compile(ast.Module([node], type_ignores=[]), file_path, "exec"), + var_context, + var_context, + ) + else: + exec( + compile( + ast.Interactive([node], type_ignores=[]), file_path, "single" + ), + var_context, + var_context, + ) + + # When we called the function to get the top level recall context manager, we didn't + # store the result in a variable and re-use that variable here. That is intentional, + # because during the evaluation of the app code, + # replace_top_level_recall_context_manager() may have been called, which swaps + # out the context manager, and it's the new one that we need to exit here. + get_top_level_recall_context_manager().__exit__(None, None, None) + + # If we're running as an Express app but there's also a top-level item named app + # which is a shiny.App object, the user probably made a mistake. + if "app" in var_context and isinstance(var_context["app"], App): + raise RuntimeError( + "This looks like a Shiny Express app because it imports shiny.express, " + "but it also looks like a Shiny Classic app because it has a variable named " + "`app` which is a shiny.App object. Remove either the shiny.express import, " + "or the app=App()." + ) - var_context: dict[str, object] = { - "__file__": file_path, - display_decorator_func_name: _display_decorator_function_def, - } + return ui_result - # Execute each top-level node in the AST - for node in tree.body: - if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): - exec( - compile(ast.Module([node], type_ignores=[]), file_path, "exec"), - var_context, - var_context, - ) - else: - exec( - compile(ast.Interactive([node], type_ignores=[]), file_path, "single"), - var_context, - var_context, - ) + except AttributeError as e: + # Need to catch AttributeError and convert to a different type of error, because + # uvicorn specifically catches AttributeErrors and prints an error message that + # is helpful for normal ASGI apps, but misleading in the case of Shiny Express. + raise RuntimeError(e) - # When we called the function to get the top level recall context manager, we didn't - # store the result in a variable and re-use that variable here. That is intentional, - # because during the evaluation of the app code, - # replace_top_level_recall_context_manager() may have been called, which swaps - # out the context manager, and it's the new one that we need to exit here. - get_top_level_recall_context_manager().__exit__(None, None, None) - - # If we're running as an Express app but there's also a top-level item named app - # which is a shiny.App object, the user probably made a mistake. - if "app" in var_context and isinstance(var_context["app"], App): - raise RuntimeError( - "This looks like a Shiny Express app because it imports shiny.express, " - "but it also looks like a Shiny Classic app because it has a variable named " - "`app` which is a shiny.App object. Remove either the shiny.express import, " - "or the app=App()." - ) - - return ui_result + finally: + sys.displayhook = prev_displayhook _top_level_recall_context_manager: RecallContextManager[Tag] From 867ddf7c5aa6316a5fd8fca9330799f52cacd4eb Mon Sep 17 00:00:00 2001 From: Winston Chang Date: Mon, 11 Dec 2023 12:51:42 -0600 Subject: [PATCH 2/2] Use exception chaining --- shiny/express/_run.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shiny/express/_run.py b/shiny/express/_run.py index d8cb923fe..995a1a560 100644 --- a/shiny/express/_run.py +++ b/shiny/express/_run.py @@ -126,7 +126,7 @@ def set_result(x: object): # Need to catch AttributeError and convert to a different type of error, because # uvicorn specifically catches AttributeErrors and prints an error message that # is helpful for normal ASGI apps, but misleading in the case of Shiny Express. - raise RuntimeError(e) + raise RuntimeError(e) from e finally: sys.displayhook = prev_displayhook