Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(shiny create): Support simpler syntax for --github flag #1623

Merged
merged 24 commits into from
Aug 21, 2024

Conversation

gadenbuie
Copy link
Collaborator

@gadenbuie gadenbuie commented Aug 20, 2024

This PR replaces the requirement for a full URL to a template hosted in a GitHub repo, replacing it with a shorter, more familiar syntax.

# In addition to the current incantation
shiny create -g https://github.com/posit-dev/py-shiny-templates/tree/main/map-distance

# These are now valid
shiny create --github posit-dev/py-shiny-templates/map-distance

shiny create --github posit-dev/py-shiny-templates/map-distance@main

Because there are many common ways to specify the ref and path portions of the GitHub spec, I ended up adding support for more than a few variations

  • {repo_owner}/{repo_name}@{ref}:{path}
  • {repo_owner}/{repo_name}:{path}@{ref}
  • {repo_owner}/{repo_name}:{path}
  • {repo_owner}/{repo_name}/{path}
  • {repo_owner}/{repo_name}/{path}@{ref}
  • {repo_owner}/{repo_name}/{path}?ref={ref}

Alternatively, the --template flag can now be combined with the --github flag, so these iterations are now the same as the above:

shiny create --github posit-dev/py-shiny-templates --template map-distance

shiny create --github posit-dev/py-shiny-templates@main --template map-distance

I'd really like to support listing available templates and asking the user to pick one, so that we could support

shiny create --github posit-dev/py-shiny-templates

with a missing --template flag. shiny create would download posit-dev/py-shiny-templates, find the available templates in the repo, and provide a menu to users.

While here, I've also improved the use of formatting and color in the CLI interface. Here's an example of the output seen when creating the map-distance template:

image

@gadenbuie gadenbuie marked this pull request as ready for review August 21, 2024 15:25
@gadenbuie gadenbuie requested a review from cpsievert August 21, 2024 15:29
shiny/_main.py Outdated Show resolved Hide resolved
shiny/_main.py Outdated Show resolved Hide resolved
shiny/_template_utils.py Outdated Show resolved Hide resolved
shiny/_template_utils.py Outdated Show resolved Hide resolved
shiny/_main.py Outdated
@@ -602,22 +605,18 @@ def create(
template: Optional[str] = None,
mode: Optional[str] = None,
github: Optional[str] = None,
dir: Optional[str | Path] = None,
dir: Optional[Path] = None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we sure this is always a Path? If so, why keep the str runtime check?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops, I didn't mean to remove str from the input typing. I do think we could use

type=click.Path(dir_okay=True, file_okay=False)

for the --dir option. This does guarantee that dir is then a Path(). I'd still be on the fence about allowing str, wdyt?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, yea, that seems sensible, and I don't see any harm in keeping the str type.

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 kept it but while looking at this realized we:

  1. Should be asking for the destination directory earlier, e.g. before asking whether they want express/core syntax
  2. Can better throw an error if the --dir is actually a file
  3. Can improve the directory_prompt() experience by making dest_dir the first argument.

064f59f

Copy link
Collaborator

@cpsievert cpsievert left a comment

Choose a reason for hiding this comment

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

LGTM once tests are passing

@gadenbuie gadenbuie enabled auto-merge (squash) August 21, 2024 21:12
@gadenbuie gadenbuie merged commit 5ecaca1 into main Aug 21, 2024
47 of 48 checks passed
@gadenbuie gadenbuie deleted the feat/create-improve-gh-spec branch August 21, 2024 23:30
schloerke added a commit that referenced this pull request Aug 22, 2024
* main:
  refactor: `shiny create` and `shiny add test` supporting functions (#1629)
  Update CHANGELOG.md
  Fix #1601: Force text/javascript for .js files
  Work around griffe 1.0 breaking changes
  feat(shiny create): Support simpler syntax for `--github` flag (#1623)
  Make sure `Chat.messages(format='google')` converts role assistant -> model (#1622)
  Fix `NormalizerRegistry.register()` (#1619)
  Update JS/CSS from shiny and bslib (#1617)
  Add ability to update `Chat()`'s input placeholder (#1594)
  docs: Add double-quotes to shiny[theme] (#1605)
  `json.dump()` each websocket message once instead of twice (#1597)
schloerke added a commit to machow/py-shiny that referenced this pull request Sep 5, 2024
* main:
  setup.cfg -> pyproject.toml (posit-dev#1625)
  docs(navset): Add server function to navset docs (posit-dev#1596)
  Pin syrupy to avoid pytest-rerunfailures incompatibility (posit-dev#1632)
  refactor: `shiny create` and `shiny add test` supporting functions (posit-dev#1629)
  Update CHANGELOG.md
  Fix posit-dev#1601: Force text/javascript for .js files
  Work around griffe 1.0 breaking changes
  feat(shiny create): Support simpler syntax for `--github` flag (posit-dev#1623)
  Make sure `Chat.messages(format='google')` converts role assistant -> model (posit-dev#1622)
  Fix `NormalizerRegistry.register()` (posit-dev#1619)
  Update JS/CSS from shiny and bslib (posit-dev#1617)
  Add ability to update `Chat()`'s input placeholder (posit-dev#1594)
  docs: Add double-quotes to shiny[theme] (posit-dev#1605)
  `json.dump()` each websocket message once instead of twice (posit-dev#1597)
  docs: Update note on style keys allowing for kebab-case (posit-dev#1595)
  docs: fix navset function links (posit-dev#1590)
  Get `ui.Chat()` working inside Shiny modules (posit-dev#1582)
  bug(notification): Allow for duration to be `None` and passed to client (posit-dev#1577)
  Fix logic for detection of support files in docs (posit-dev#1580)
  docs(navset): Add examples for navset (posit-dev#1579)
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.

2 participants