Skip to content

fix: preserve path separators in create command argument#526

Open
srtaalej wants to merge 12 commits into
mainfrom
ale-fix-create-path-regression
Open

fix: preserve path separators in create command argument#526
srtaalej wants to merge 12 commits into
mainfrom
ale-fix-create-path-regression

Conversation

@srtaalej
Copy link
Copy Markdown
Contributor

@srtaalej srtaalej commented May 7, 2026

Changelog

The create command preserves separators once again if direction to a nested path is provided.

Summary

  • Fixes the slack create path/to/my-app regression where path separators were being replaced with dashes, creating a flat path-to-my-app directory instead of preserving the path/to/my-app structure
  • Adds parseAppPath helper that splits user input into path prefix + basename, only kebab-casing the basename
  • Display name for manifests is derived from the raw basename (not the full path)

Behavior

$ slack create my-app              # dir: ./my-app/        manifest: "my-app"
$ slack create "My App"            # dir: ./my-app/        manifest: "My App"
$ slack create path/to/my-app      # dir: ./path/to/my-app/ manifest: "my-app"
$ slack create path/to/My App      # dir: ./path/to/my-app/ manifest: "My App"

Stacks on #521 which handles the display name vs dir name separation.

Test plan

  • TestParseAppPath — 12 cases covering simple names, paths, absolute paths, dot-prefixed, trailing slashes, and error cases
  • Existing TestGetProjectDirectoryName still passes (single-component names)
  • Full test suite (go test ./...) passes
  • Manual: slack create path/to/my-app with existing path/to/ directory → verify nested structure created and manifest has my-app
  • Manual: slack create "My App" → verify dir is my-app, manifest display is My App

srtaalej and others added 5 commits May 4, 2026 16:04
The display_information.name and bot_user.display_name fields in manifest
files were set to the kebab-case directory name (e.g. "my-app") instead of
a human-readable title case name (e.g. "My App").
…ebab

Addresses review feedback — instead of converting the kebab-case
directory name to title case (which mangles names like "TIM" or
"Translator - Translate Languages"), pass the original user-provided
app name through to manifest display fields. This preserves the
user's exact casing and formatting.
Co-authored-by: Eden Zimbelman <eden.zimbelman@salesforce.com>
The getAppDirName function was normalizing the entire input including
path separators, so `slack create path/to/my-app` produced a flat
directory `path-to-my-app` instead of preserving the path structure.

Adds parseAppPath which splits user input into path prefix and basename,
only kebab-casing the basename. The display name for manifests is
derived from the raw basename (preserving original casing).

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
@srtaalej srtaalej requested a review from a team as a code owner May 7, 2026 21:16
@srtaalej srtaalej added bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented semver:patch Use on pull requests to describe the release version increment labels May 7, 2026
@srtaalej srtaalej self-assigned this May 7, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 7, 2026

Codecov Report

❌ Patch coverage is 73.33333% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.31%. Comparing base (bd6a088) to head (cf6a674).

Files with missing lines Patch % Lines
internal/pkg/create/create.go 61.90% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #526      +/-   ##
==========================================
+ Coverage   71.30%   71.31%   +0.01%     
==========================================
  Files         222      222              
  Lines       18706    18725      +19     
==========================================
+ Hits        13338    13354      +16     
- Misses       4188     4190       +2     
- Partials     1180     1181       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Base automatically changed from ale-fix-display-name-casing to main May 8, 2026 15:22
@zimeg zimeg added the changelog Use on updates to be included in the release notes label May 11, 2026
@zimeg zimeg added this to the Next Release milestone May 11, 2026
Comment thread internal/iostreams/forms_test.go Outdated
@zimeg zimeg self-requested a review May 11, 2026 16:48
Copy link
Copy Markdown
Member

@zimeg zimeg left a comment

Choose a reason for hiding this comment

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

@srtaalej Thank you for bringing a fix to PR! I'm leaving a comment of another case to consider with this but I'm eager to get this merged 🚀

Comment on lines 224 to 229
// Verify that the parent directory path exists (we will not create missing directories)
if exists, err := parentDirExists(dirPath); err != nil {
return "", err
} else if !exists {
return "", slackerror.New(slackerror.ErrAppDirectoryAccess)
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🔬 thought: So curious! I thought creating missing directories was supported but I am wrong!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes very curious! i thought so too but i think it would be nice to have 🤔 perhaps in followups 🚀

// Get the app selection and accompanying app directory name (this may change when we find the unique directory name)
appDirName, err := getAppDirName(createArgs.AppName)
// Parse the app name input into a directory path and display name
appPath, displayName, err := parseAppPath(createArgs.AppName)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🐛 issue: I'm finding the app path can be changed with the "--name" flag which is unexpected as:

$ slack create path/to/my-app --name "APPPP"

📂 Created a new Slack project
   Cloning template slack-samples/bolt-js-starter-template
   To path ~/programming/tools/slack-cli/apppp

🔍 thought: I'd instead expect the argument path to be preferred here with the name being applied to the app itself? I'm unsure if separating these functions makes this more clear but think this case is good to test too!

Copy link
Copy Markdown
Contributor Author

@srtaalej srtaalej May 11, 2026

Choose a reason for hiding this comment

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

hmmm i thought the --name flag was always preferred? this was introduced with the name flag in #327
but thanks for catching this because i would also expect --name to override the display name and leave the path alone

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@srtaalej Ahh we might be finding nuance of this command 😓 I recall these discussion too and didn't account for these edges...

The --name flag now only sets the manifest display name, preserving
any path provided via the positional argument for directory placement.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented changelog Use on updates to be included in the release notes semver:patch Use on pull requests to describe the release version increment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants