fix: preserve original app name as manifest display name#521
Conversation
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").
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #521 +/- ##
==========================================
- Coverage 71.30% 71.27% -0.04%
==========================================
Files 222 222
Lines 18677 18678 +1
==========================================
- Hits 13318 13313 -5
- Misses 4184 4185 +1
- Partials 1175 1180 +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.
@srtaalej Thanks for getting this started 👾 ✨
I'm hesitant on this approach and am wondering if we instead separate how arguments and flags are parsed at this point?
Some issues I ran into are in comments but I'd like to share how I think about the "create" command:
$ slack create # Generates a random app name as current
$ slack create my-app # Keeps path "my-app" and manifest "my-app"
$ slack create "my App" # Uses path "my-app" and manifest "my App"🧰 Additional cases might complicate this but it's a good time to untangle this perhaps? I'm curious also if adding the --outdir flag makes parts of this more clear, while still supporting the happiest path simple cases?
|
|
||
| // UpdateDefaultProjectFiles should update any project specific files if any | ||
| func UpdateDefaultProjectFiles(fs afero.Fs, dirPath string, appDirName string) error { | ||
| displayName := kebabToTitleCase(appDirName) |
There was a problem hiding this comment.
🔍 issue: We might want to avoid changing the app name format altogether if this is provided in flags? Forgive me if I'm not understanding requirements right, but this app manifest for example might break with replacements:
Translator - Translate Languages
🔗 https://slack.com/marketplace/A8KHN4EDV-translator-translate-languages
|
|
||
| // UpdateDefaultProjectFiles should update any project specific files if any | ||
| func UpdateDefaultProjectFiles(fs afero.Fs, dirPath string, appDirName string) error { | ||
| displayName := kebabToTitleCase(appDirName) |
There was a problem hiding this comment.
🐍 ramble: Apps I keep might not fare well for additional example: "TIM", "TOM", or "snaek" have strange cases.
…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.
zimeg
left a comment
There was a problem hiding this comment.
@srtaalej This is running alright but I'm not certain the expected behavior matches the PR description and title since IIRC we're now avoiding title case defaults?
Leaving a few thoughts before approving and I also think a changelog toward this is best? Perhaps a standalone "bug" label too unless new featuresets are included too?
|
|
||
| // Update default project files' app name, bot name, etc | ||
| if err := app.UpdateDefaultProjectFiles(clients.Fs, projectDirPath, appDirName); err != nil { | ||
| if err := app.UpdateDefaultProjectFiles(clients.Fs, projectDirPath, appDirName, createArgs.AppName); err != nil { |
There was a problem hiding this comment.
🌟 praise: Nice! Keeping these separate makes it much easier to think about for me!
| {"package.json", regexReplaceAppNameInPackageJSON, appDirName}, | ||
| {"pyproject.toml", regexReplaceAppNameInPyprojectToml, appDirName}, |
There was a problem hiding this comment.
🧠 praise: Nice separation for project titles!
Co-authored-by: Eden Zimbelman <eden.zimbelman@salesforce.com>
|
@zimeg updated the pr description and tests! |
| "WriteFile error": { | ||
| appDirName: "vibrant-butterfly-1234", | ||
| existingFiles: map[string]string{ | ||
| "manifest.json": string(testdata.ManifestJSON), | ||
| }, | ||
| expectedFiles: map[string]string{ | ||
| "manifest.json": string(testdata.ManifestJSONAppName), | ||
| }, | ||
| expectedErrorType: nil, | ||
| }, |
There was a problem hiding this comment.
📠 question: Was this case patched? It's not clear to me why the test is removed?
There was a problem hiding this comment.
that test case is identical to the "manifest.json file exists" test case - i removed it because it was redundant
There was a problem hiding this comment.
i will add a test case that actually produces the write file error separately
Changelog
Fix: App manifest display names now preserve the original app name instead of using the kebab-case directory name during
slack create.Summary
display_information.name,bot_user.display_name, and Deno SDKManifest({ name })) now preserve the user's original input instead of using the kebab-case directory namepackage.jsonandpyproject.tomlnames remain kebab-case per npm/Python conventionsBehavior
Test plan
Test_App_UpdateDefaultProjectFilesvalidates manifest files get the display name while package.json/pyproject.toml keep kebab-caseTest_App_UpdateDefaultProjectFiles_WriteFileErrorcovers the WriteFile error pathTest_RegexReplaceAppNameInManifestupdated to reflect display name inputslack create "My App"→ verify manifest display name is "My App", directory ismy-app/, and package.json staysmy-app