feat(create): show generated app name in hint line instead of input default#340
feat(create): show generated app name in hint line instead of input default#340
Conversation
srtaalej
added
enhancement
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #340 +/- ##
==========================================
+ Coverage 64.73% 64.78% +0.05%
==========================================
Files 212 212
Lines 17809 17823 +14
==========================================
+ Hits 11528 11546 +18
+ Misses 5207 5202 -5
- Partials 1074 1075 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
srtaalej
changed the title
zimeg
left a comment
zimeg
left a comment
There was a problem hiding this comment.
@srtaalej LGTM and I think a kind change to the create experience - revising the generated name after a template is cloned is quite burdensome I've found!
Approving now with the logic working well, but I left a few thoughts about how we're organizing this code. Also a note on prompts, but nothing so blocking! 🚀
internal/pkg/create/create.go
Outdated
| // GenerateRandomAppName will create a random app name based on two words and a number | ||
| func GenerateRandomAppName() string { |
There was a problem hiding this comment.
🪬 question: Can we move this logic into cmd/project/create.go instead of exporting from pkg? I'm hoping we can move all of these "inputs" to the command level as we move away from internal/pkg long-term!
There was a problem hiding this comment.
⭐ ramble: We might prefer erroring below - L165 - instead of attempting to generate this too!
There was a problem hiding this comment.
🤖 ramble: Also, so unrelated, but we might want to replace " " spaces with "-" in that getAppDirName function... While still removing surrounding spaces of course!
It's strange to combine words otherwise IMHO!
internal/iostreams/survey.go
Outdated
| type InputPromptConfig struct { | ||
| Required bool // Whether the input must be non-empty | ||
| Required bool // Whether the input must be non-empty | ||
| Default string // Default value pre-filled in the prompt |
There was a problem hiding this comment.
👁️🗨️ question: Is this used in the defaulting prompt logic above?
No blocker but I'm hoping we can move toward huh instead of updating these packages overall!
There was a problem hiding this comment.
good catch - default is not used! missed that before committing 😬
zimeg
left a comment
zimeg
left a comment
There was a problem hiding this comment.
Edit: I notice we're not guarding against the CI case!
$ slack create -t slack-samples/bolt-js-starter-template | cat
We should avoid prompting for this - I'll leave a note with ideas next!
zimeg
left a comment
zimeg
left a comment
There was a problem hiding this comment.
📫 Here's the comment on interactive inputs!
Our current input prompt implementation doesn't seem to error as we'd hope for this input, but let's guard against this at the callsite instead of updating prompts further I think!
Co-authored-by: Eden Zimbelman <eden.zimbelman@salesforce.com>
zimeg
left a comment
zimeg
left a comment
There was a problem hiding this comment.
@srtaalej I'm perhaps too soon to review again, but want to callout the test case we want to protect before merging this changeset, perhaps with unit test:
$ lack create -t slack-samples/bolt-js-starter-template | cat
Check /Users/eden.zimbelman/.slack/logs/slack-debug-20260218.log for error logs
app name is required
Instead of erroring for this command we can fallback to the default generated name as requiring new arguments might break existing scripts 👾
| "creates a bolt application from prompts": { | ||
| Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { | ||
| cm.IO.On("IsTTY").Return(true) |
There was a problem hiding this comment.
🧪 suggestion: We might want to include a test case alongside af97eca changes - do we have a unit test that's checking for --template arguments while IsTTY is also set to false?
| // trim whitespace | ||
| appName = strings.ReplaceAll(appName, " ", "") | ||
| appName = strings.TrimSpace(appName) | ||
| appName = strings.ReplaceAll(appName, " ", "-") |
There was a problem hiding this comment.
📣 note: Let's call this out in the changelog as a fix as well! I'll make a few changes to this since we'll also want to keep it focused on just developer-facing features-
|
🗣️ ramble: We could leave a note for later if that is a breaking change we'll want - sharing this for quick reference: |
2 participants
Changelog
The
slack createcommand now prompts for a custom app name if the randomly generated default isn't quite right for whatever reason. We prompt this before scaffolding the new project from a template!When making new project directories with a custom app name, we now replace spaces with a dash (instead of removing spaces altogether) to make navigation a bit better across systems. 🦋
Fixes
slack createwith an app name containing spaces (e.g."my cool app") now producesmy-cool-appinstead ofmycoolapp. Leading and trailing whitespace is also trimmed.slack create -t <template> | cat(or in scripts) no longer errors with "app name is required". A random name is generated automatically when no name is provided and the terminal is non-interactive.Summary
This PR enables users to input an app name if one wasn't provided on
slack create. Generated name can still be used by pressingEnter.Test plan
make test testdir=cmd/project testname=TestCreateCommandpassesmake test testdir=internal/pkg/create testname=TestGetProjectDirectoryNamepasses./bin/slack createshows generated name in gray hint, input field is empty./bin/slack create -t slack-samples/bolt-js-starter-template | catgenerates a default name instead of erroringRequirements