Skip to content

Conversation

wch
Copy link
Collaborator

@wch wch commented Mar 15, 2024

Intent

This PR adds support for shiny mode magic comments which are generated by interactive Quarto dashboards that use Shiny for Python. This change allows users to create Quarto dashboards that have from shiny.express import ... instead of from shiny ...; previously those apps would not run.

See the following for more information:

Type of Change

  • Bug Fix
  • New Feature
  • Breaking Change

Approach

This copies over code from the py-shiny repository, from this file:

https://github.com/posit-dev/py-shiny/blob/2b66c898cb9d0fc827e587b11a839e270b2e814f/shiny/express/_is_express.py

Directions for Reviewers

Checklist

  • I have updated CHANGELOG.md to cover notable changes.
  • I have updated all related GitHub issues to reflect their current state.

@wch wch force-pushed the shiny-is-express branch from 0945a23 to b5004f1 Compare March 15, 2024 15:47
@wch wch marked this pull request as ready for review March 15, 2024 16:13
@wch
Copy link
Collaborator Author

wch commented Mar 15, 2024

The linting test is failing because in Python 3.7, the typing module doesn't have Literal. (It is present in 3.8+.)

I could work around this by importing from typing_extensions. That said, my understanding is that we've dropped Python 3.7 support, so can we stop testing on that platform?

Copy link

github-actions bot commented Mar 15, 2024

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
4558 3274 72% 0% 🟢

New Files

No new covered files...

Modified Files

File Coverage Status
rsconnect/shiny_express.py 75% 🟢
TOTAL 75% 🟢

updated for commit: 3126880 by action🐍

@mmarchetti
Copy link
Contributor

rsconnect-python hasn't removed Python 3.7 support, but Connect has removed it so it's probably time.

@nealrichardson
Copy link
Contributor

rsconnect-python hasn't removed Python 3.7 support, but Connect has removed it so it's probably time.

I made #556 for that.

pass


def find_magic_comment_mode(content: str) -> Literal["core", "express"] | None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worth adding a test for this function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added tests for Shiny Express app detection in general in cc3766e.

@aronatkins
Copy link
Collaborator

@wch - if you pull in master, you should see CI pass.

@wch wch merged commit 164ec4c into master Mar 21, 2024
@wch wch deleted the shiny-is-express branch March 21, 2024 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants