[SNOW-1039218] feat: add hidden app list-templates command#887
[SNOW-1039218] feat: add hidden app list-templates command#887sfc-gh-mchok merged 17 commits intomainfrom
Conversation
| with SecurePath.temporary_directory() as temp_path: | ||
| from git import Repo | ||
|
|
||
| repo = Repo.clone_from( | ||
| url=OFFICIAL_TEMPLATES_GITHUB_URL, | ||
| to_path=temp_path.path, | ||
| filter=["tree:0"], | ||
| depth=1, | ||
| ) |
There was a problem hiding this comment.
IMHO we should reuse the logic from the init command instead of reimplementing it here. You'll probably have to factor it out first though.
There was a problem hiding this comment.
Good callout, I extracted a function to src/snowflake/cli/plugins/nativeapp/utils.py, let me know if that works for you!
| "file_content, expected_paragraph", | ||
| [ | ||
| ( | ||
| """ |
There was a problem hiding this comment.
Suggest using dedent here
There was a problem hiding this comment.
I tried and wasn't able to make it work smoothly since some of the whitespace is crucial in how the function works (e.g. first paragraph needs to have no whitespace in between), but I turned the multiline strings into multi line-strings to have a similar effect where the file isn't as long, let me know if that works for you or if I massively misunderstood the assignment 😅
EDIT: Scratch that, pretty sure I misunderstood, I changed it to use dedent now, thank you!
| # get the template descriptions from the README.md in its directory | ||
| template_descriptions = [ | ||
| get_first_paragraph_from_markdown_file( | ||
| (temp_path / directory / "README.md").path |
There was a problem hiding this comment.
Right now, this will fail if any templates are created that don't have top-level README.md files. I suggest we provide a fallback to None both in this case and when there is no paragraph text in the readme
There was a problem hiding this comment.
Good callout - added a test for it as well, thanks!
sfc-gh-cgorrie
left a comment
sfc-gh-cgorrie
left a comment
There was a problem hiding this comment.
I'd like to see the helper method throw an exception rather than silently returning None if the file doesn't exist, and then split out the negative test cases in two:
- file actually doesn't exist
- file doesn't have a non-empty regular paragraph line
| @app.command("list-templates", hidden=True) | ||
| def app_list_templates(**options) -> CommandResult: | ||
| """ | ||
| Prints information regarding templates that can be used with snow app init. |
There was a problem hiding this comment.
Nit: "... regarding the official templates ...", so it is clear that only our Github templates are listed in this command.
| ] | ||
|
|
||
| result = ( | ||
| {"template": directory, "description": description} |
There was a problem hiding this comment.
I really like the idea of a little description along with template names!
sfc-gh-bgoel
left a comment
sfc-gh-bgoel
left a comment
There was a problem hiding this comment.
LGTM once the small change can be made to the description.
sfc-gh-mchok
dismissed stale reviews from sfc-gh-bgoel and sfc-gh-cgorrie
via
34f4126
| ) | ||
|
|
||
|
|
||
| @app.command("list-templates", hidden=True) |
There was a problem hiding this comment.
Today we have feature flags. Have you consider using them?
There was a problem hiding this comment.
I believe this was clarified offline, please see the slack thread of the PR for the full discussion, but to summarize:
- For now, we intend to make it available for our IDE plugin, but not expose directly it to users
- We don't want users to have to enable a feature flag before the IDE plugin can work
- It doesn't fit the use case of an experimental flag since this command doesn't change the behaviour of any existing commands nor is it interacting with Snowflake
Please let me know if this works for you :)
* [SNOW-1039218] feat: add hidden app list-templates command * [SNOW-1039218] feat: factor out shallow clone and add None fallback to markdown util * [SNOW-1039218] refactor: change union typing for version support * [SNOW-1039218] test: adjust test parameters with dedent * [SNOW-1039218] fix: only import git locally * [SNOW-1039218] fix: PathLike not subscriptable * [SNOW-1039218] feat: raise Exception when file not found * [SNOW-1039218] docs: clarify only official templates for command * [SNOW-1039218] test: update snapshot for new help message
6 participants
Pre-review checklist
Changes description
Added a
snow app list-templateshidden command to support dynamic fetching of official Native App templates inside our PyCharm pluginFor more information please see: https://snowflakecomputing.atlassian.net/browse/SNOW-1039218