-
Notifications
You must be signed in to change notification settings - Fork 31
fix: preserve path separators in create command argument #526
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
base: main
Are you sure you want to change the base?
Changes from all commits
7e47fee
af8e3f4
e0c3bb0
39cdaad
ac8fcea
eed95a8
3ed0b6f
fca83ad
8c6c53b
1026d94
bdeadda
cf6a674
7e4d486
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -116,10 +116,16 @@ func runCreateCommand(clients *shared.ClientFactory, cmd *cobra.Command, args [] | |||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| // --name flag overrides any positional app name argument | ||||||||||||||||||||||||||||||||||||||||
| // This allows users to name their app "agent" without triggering the AI Agent shortcut | ||||||||||||||||||||||||||||||||||||||||
| // --name flag overrides the manifest display name but preserves any path | ||||||||||||||||||||||||||||||||||||||||
| // from the positional argument. When no positional arg is given (e.g. | ||||||||||||||||||||||||||||||||||||||||
| // "slack create --name APPPP"), the name flag also becomes the directory | ||||||||||||||||||||||||||||||||||||||||
| // path since there's nothing else to derive it from. | ||||||||||||||||||||||||||||||||||||||||
| displayNameOverride := "" | ||||||||||||||||||||||||||||||||||||||||
| if nameFlagProvided { | ||||||||||||||||||||||||||||||||||||||||
| appNameArg = createAppNameFlag | ||||||||||||||||||||||||||||||||||||||||
| displayNameOverride = createAppNameFlag | ||||||||||||||||||||||||||||||||||||||||
| if appNameArg == "" { | ||||||||||||||||||||||||||||||||||||||||
| appNameArg = createAppNameFlag | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+119
to
+128
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🪴 suggestion: We might want to move this alongside logic below with some shuffling: slack-cli/cmd/project/create.go Lines 148 to 166 in 7e4d486
|
||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| // List templates and exit early if the --list flag is set | ||||||||||||||||||||||||||||||||||||||||
|
|
@@ -164,10 +170,11 @@ func runCreateCommand(clients *shared.ClientFactory, cmd *cobra.Command, args [] | |||||||||||||||||||||||||||||||||||||||
| subdir = template.GetSubdir() | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| createArgs := create.CreateArgs{ | ||||||||||||||||||||||||||||||||||||||||
| AppName: appNameArg, | ||||||||||||||||||||||||||||||||||||||||
| Template: template, | ||||||||||||||||||||||||||||||||||||||||
| GitBranch: createGitBranchFlag, | ||||||||||||||||||||||||||||||||||||||||
| Subdir: subdir, | ||||||||||||||||||||||||||||||||||||||||
| AppName: appNameArg, | ||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🪬 thought: The meaning of these arguments has inverted for me and I now think about this variable as the app path that might be used for the app name defaults. The path separators of this PR must have inspired such idea! 🔮 suggestion(follow-up): We might find it useful to update this but I'd like to know if I'm thinking about this right?
Suggested change
|
||||||||||||||||||||||||||||||||||||||||
| DisplayName: displayNameOverride, | ||||||||||||||||||||||||||||||||||||||||
| Template: template, | ||||||||||||||||||||||||||||||||||||||||
| GitBranch: createGitBranchFlag, | ||||||||||||||||||||||||||||||||||||||||
| Subdir: subdir, | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| clients.EventTracker.SetAppTemplate(template.GetTemplatePath()) | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,10 +50,11 @@ var copyIgnoreFiles = []string{".DS_Store"} | |
|
|
||
| // CreateArgs are the arguments passed into the Create function | ||
| type CreateArgs struct { | ||
| AppName string | ||
| Template Template | ||
| GitBranch string | ||
| Subdir string | ||
| AppName string | ||
| DisplayName string | ||
| Template Template | ||
| GitBranch string | ||
| Subdir string | ||
| } | ||
|
|
||
| // Create will create a new Slack app on the file system and app manifest on the Slack API. | ||
|
|
@@ -67,16 +68,21 @@ func Create(ctx context.Context, clients *shared.ClientFactory, createArgs Creat | |
| return "", slackerror.Wrap(err, slackerror.ErrAppDirectoryAccess) | ||
| } | ||
|
|
||
| // 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) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: 🔍 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!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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... |
||
| if err != nil { | ||
| return "", slackerror.Wrap(err, slackerror.ErrAppDirectoryAccess) | ||
| } | ||
|
|
||
| // --name flag overrides only the display name, preserving the path from the argument | ||
| if createArgs.DisplayName != "" { | ||
| displayName = createArgs.DisplayName | ||
| } | ||
|
|
||
| // Get the project's full directory path | ||
| projectDirPath := "" | ||
| if filepath.IsLocal(appDirName) { | ||
| projectDirPath = filepath.Join(workingDirPath, appDirName) | ||
| if filepath.IsLocal(appPath) { | ||
| projectDirPath = filepath.Join(workingDirPath, appPath) | ||
| projectDirPath, err = getAvailableDir(ctx, projectDirPath) | ||
| if err != nil { | ||
| return "", slackerror.Wrap(err, slackerror.ErrAppDirectoryAccess) | ||
|
|
@@ -86,7 +92,7 @@ func Create(ctx context.Context, clients *shared.ClientFactory, createArgs Creat | |
| return "", slackerror.Wrap(err, slackerror.ErrAppDirectoryAccess) | ||
| } | ||
| } else { | ||
| projectDirPath = filepath.Join(appDirName) | ||
| projectDirPath = filepath.Join(appPath) | ||
| projectDirPath, err = getAvailableDir(ctx, projectDirPath) | ||
| if err != nil { | ||
| return "", slackerror.Wrap(err, slackerror.ErrAppDirectoryAccess) | ||
|
|
@@ -98,7 +104,7 @@ func Create(ctx context.Context, clients *shared.ClientFactory, createArgs Creat | |
| } | ||
|
|
||
| // Update the app's directory name now that the unique directory is created | ||
| appDirName = filepath.Base(projectDirPath) | ||
| appDirName := filepath.Base(projectDirPath) | ||
|
|
||
| // Print a bunch of information about the progress of the command to traces | ||
| // and debugs and the standard output here | ||
|
|
@@ -150,7 +156,7 @@ func Create(ctx context.Context, clients *shared.ClientFactory, createArgs Creat | |
| }() | ||
|
|
||
| // Update default project files' app name, bot name, etc | ||
| if err := app.UpdateDefaultProjectFiles(clients.Fs, projectDirPath, appDirName, createArgs.AppName); err != nil { | ||
| if err := app.UpdateDefaultProjectFiles(clients.Fs, projectDirPath, appDirName, displayName); err != nil { | ||
| return "", slackerror.Wrap(err, slackerror.ErrProjectFileUpdate) | ||
| } | ||
|
|
||
|
|
@@ -192,6 +198,29 @@ func getAppDirName(appName string) (string, error) { | |
| return appName, nil | ||
| } | ||
|
|
||
| // parseAppPath splits user input into a directory path (with kebab-cased basename) | ||
| // and a display name (the raw basename preserving original casing/spacing). | ||
| func parseAppPath(input string) (appPath string, displayName string, err error) { | ||
| input = strings.TrimSpace(input) | ||
| if input == "" { | ||
| return "", "", fmt.Errorf("app name is required") | ||
| } | ||
|
|
||
| input = filepath.Clean(input) | ||
| displayName = filepath.Base(input) | ||
| pathPrefix := filepath.Dir(input) | ||
|
|
||
| dirName, err := getAppDirName(displayName) | ||
| if err != nil { | ||
| return "", "", err | ||
| } | ||
|
|
||
| if pathPrefix == "." { | ||
| return dirName, displayName, nil | ||
| } | ||
| return filepath.Join(pathPrefix, dirName), displayName, nil | ||
| } | ||
|
|
||
| // getAvailableDir will return a unique directory path. | ||
| // If dirPath already exists, then a unique numbered path will be appended to the path. | ||
| func getAvailableDir(ctx context.Context, dirPath string) (string, error) { | ||
|
|
||
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.
🔬 suggestion: I understand within
pkgthis overrides a parsed path but that term might be confusing from this calling code.