Add support for Shiny Express apps when writing manifest files #558
Conversation
wch
force-pushed
the
shiny-express-manifest
branch
from
cfb2840 to
a638e5d
Compare
wch
force-pushed
the
shiny-express-manifest
branch
from
a638e5d to
14fd553
Compare
wch
force-pushed
the
shiny-express-manifest
branch
from
7dc0290 to
96e243a
Compare
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
rsconnect/main.py
Outdated
| failed("Error: " + str(exc)) | ||
| except Exception as exc: | ||
| if click.get_current_context("verbose"): | ||
| if click.get_current_context(True): |
There was a problem hiding this comment.
I believe this was an existing bug, which mypy started noticing after I made some other changes.
According to the docs, click.get_current_context wants a boolean, not a string. If False, it will throw an error if there's no context; if True, it'll return None in that situation. I set it to True because I think the intent here was not to throw another error.
There was a problem hiding this comment.
Let's drop the if condition entirely and always print the traceback in this case. It looks like the original intent was to check the verbose flag from the command line and only print the traceback in verbose mode. The checks for RSConnectException and EnvironmentException suppress tracebacks for known kinds of exceptions (where we provide meaningful error messages); the rest can have trackbacks for debugging.
| if sys.version_info >= (3, 10): | ||
| from typing import ParamSpec | ||
| else: | ||
| from typing_extensions import ParamSpec |
There was a problem hiding this comment.
It is possible to just import from typing_extensions, even for newer versions of Python, but I made this import conditional on version so that it'll be clear that the condition can be removed in the future when 3.10 support is dropped.
| name: Optional[str], | ||
| server: Optional[str], | ||
| api_key: Optional[str], | ||
| insecure: bool, | ||
| cacert: str, | ||
| cacert: Optional[str], | ||
| static: bool, | ||
| new: bool, | ||
| app_id: str, | ||
| title: str, | ||
| python, | ||
| force_generate, | ||
| app_id: Optional[str], | ||
| title: Optional[str], | ||
| python: Optional[str], | ||
| force_generate: bool, | ||
| verbose: int, | ||
| file: str, | ||
| extra_files, | ||
| extra_files: Sequence[str], | ||
| hide_all_input: bool, | ||
| hide_tagged_input: bool, | ||
| env_vars: typing.Dict[str, str], | ||
| image: str, | ||
| disable_env_management: bool, | ||
| env_management_py: bool, | ||
| env_management_r: bool, | ||
| env_vars: dict[str, str], | ||
| image: Optional[str], | ||
| disable_env_management: Optional[bool], | ||
| env_management_py: Optional[bool], | ||
| env_management_r: Optional[bool], |
There was a problem hiding this comment.
In general I've changed these annotations to reflect the actual possible values of what click will pass to the function. This is true here and with the other functions.
| if app_mode == AppModes.PYTHON_SHINY: | ||
| with cli_feedback("Inspecting Shiny for Python app"): | ||
| if is_express_app(entrypoint + ".py", directory): | ||
| entrypoint = "shiny.express.app:" + escape_to_var_name(entrypoint + ".py") | ||
|
|
There was a problem hiding this comment.
This is the part that adds support for Shiny Express when writing manifest files.
rsconnect/main.py
Outdated
| failed("Error: " + str(exc)) | ||
| except Exception as exc: | ||
| if click.get_current_context("verbose"): | ||
| if click.get_current_context(True): |
There was a problem hiding this comment.
Let's drop the if condition entirely and always print the traceback in this case. It looks like the original intent was to check the verbose flag from the command line and only print the traceback in verbose mode. The checks for RSConnectException and EnvironmentException suppress tracebacks for known kinds of exceptions (where we provide meaningful error messages); the rest can have trackbacks for debugging.
This PR does the following:
rsconnect deploy, only try to detect Shiny Express for Shiny apps. (Previously it would attempt the detection for other types of apps as well, includingapi,streamlit,voila, etc.)Intent
This addresses item (2) in posit-dev/py-shiny#1232:
rsconnect write-manifest shinydoes not support Shiny Express apps.Type of Change
Approach
Automated Tests
Directions for Reviewers
Checklist