-
Notifications
You must be signed in to change notification settings - Fork 24
feat: include bolt for javascript and python in the 'samples' command #139
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #139 +/- ##
==========================================
- Coverage 63.59% 63.53% -0.07%
==========================================
Files 212 212
Lines 22348 22395 +47
==========================================
+ Hits 14213 14229 +16
- Misses 7054 7080 +26
- Partials 1081 1086 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zimeg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 A few notes on changed selections adjacent to prompts for the most kind reviewers!
| selection, err := clients.IO.SelectPrompt(ctx, msg, surveyPromptOptions, | ||
| iostreams.SelectPromptConfig{ | ||
| Flag: clients.Config.Flags.Lookup("name"), | ||
| Required: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔍 The required option defaulting to false will cause this prompt to skip from the changes below!
| if opt == nil { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👁️🗨️ Here we avoid checking nonexistent flags for a changed value which is useful since the create command doesn't provide a "language" flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting how the survey is kinda getting in the way. Thanks for improving it!
mwbrooks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Implementation looks crisp and it works well! This is a very nice improvement!
🧪 Local testing works like a charm!
📝 My suggestion is to use --language <deno|node|python> to align the values with our runtime values but use a more familiar flag name. We may want to swap "Runtime" with "Language" in the Doctor command and verbose logs as well, eventually.
| fmt.Sprintf("Bolt for JavaScript %s", style.Secondary("Node.js")), | ||
| fmt.Sprintf("Bolt for Python %s", style.Secondary("Python")), | ||
| fmt.Sprintf("Deno Slack SDK %s", style.Secondary("Deno")), | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
praise: Great consideration for detail by choosing to match the styling of the create command! 🎨
cmd/project/samples.go
Outdated
|
|
||
| cmd.Flags().StringVarP(&samplesTemplateURLFlag, "template", "t", "", "template URL for your app") | ||
| cmd.Flags().StringVarP(&samplesGitBranchFlag, "branch", "b", "", "name of git branch to checkout") | ||
| cmd.Flags().StringVar(&samplesLanguageFlag, "language", "", "framework from the app runtime\n ex: \"bolt-js\", \"bolt-python\", \"deno\"") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: I like the --language flag. It's more familiar than --runtime, which we went with prior to supporting Bolt.
However, I can't shake that 2 choices are frameworks and 1 choice in a runtime. None are actually languages. 😬
I like the goal of keeping things consistent and recognizing that the create command uses language. The create command is also flawed, because it's listing Frameworks (Bolt JS, Bolt Python, and Deno SDK) and Runtimes (Node.js, Python, and Deno).
To make matter worse, the hooks return a runtime value ("deno", "node", & "python"). This at least returns the actual runtimes, haha. And the doctor command displays these values under the title "Runtime".
Zooming out, perhaps --runtime <deno|node|python> is the technically correct and aligns the best with our current use of Runtime. While --language <deno|bolt-js|bolt-python> is more familiar to developers.
🧠 My opinion is to go with --language <deno|node|python>. This is a mix of both, but the reason is 3 fold:
- If you ask a dev what language they prefer, many will say "Node" if they actually mean JavaScript or TypeScript running outside of the browser.
- The values align with our Runtime values returned by the hooks, displayed by Doctor, and displayed as secondary by the
createandsamplesprompts. - Since the values match, we can always support
--runtimeas an flag alias using asemver:minor- in the future, if we decide runtime was the better choice. Escape Hatch.
| if opt == nil { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting how the survey is kinda getting in the way. Thanks for improving it!
|
@mwbrooks These are great suggestions! 🙏
I was hoping to match The "runtime" option is so interesting to me though and wasn't something I considered! In later reworks I'll keep this in mind but for now I also find it to have more of an internal meaning.
This was a frustrated finding in an otherwise unrelated change but I hope we can revisit the cause of this soon 👁️🗨️ I will merge this PR now with the above changes. Thanks so much for the kind review and thoughts shared 🧠 ✨ |
Summary
This PR adds Bolt for JavaScript and Bolt for Python sample apps to the
samplescommand.Preview
samples.mov
Reviewers
Create more apps with these commands:
Notes
--languageflag was named to match the current similar prompt of thecreatecommand, but I'm open to updating this if another option is better!--languageflag wasn't added to thecreatecommand to avoid filtering the curated apps. IMO it might be confusing to have with that command too 🤖Requirements