Skip to content

feat: display manifest source in human-friendly format #90

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.

Sign up for GitHub

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

Merged
mwbrooks merged 6 commits into from
May 14, 2025
Merged

feat: display manifest source in human-friendly format #90

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
31559f0
feat: display manifest source in human-readable format
mwbrooks May 13, 2025
5c27011
test: fix failing tests
mwbrooks May 14, 2025
acd2a00
Update cmd/app/link.go
mwbrooks May 14, 2025
3913b13
feat: handle unknown manifest source
mwbrooks May 14, 2025
4eaea06
feat: quote the human-friendly name
mwbrooks May 14, 2025
9329993
Merge branch 'main' into mwbrooks-manifest-source-human
mwbrooks May 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions cmd/app/link.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,14 +173,15 @@ func LinkExistingApp(ctx context.Context, clients *shared.ClientFactory, app *ty
Emoji: "warning",
Text: "Warning",
Secondary: []string{
"Existing apps have manifests configured by app settings",
"Linking existing apps requires the manifest source to be " + config.ManifestSourceRemote.String(),
fmt.Sprintf(`Manifest source can be "%s" or "%s"`, config.ManifestSourceLocal.String(), config.ManifestSourceRemote.String()),
"Linking an existing app requires the app manifest source to be managed by",
fmt.Sprintf("%s.", config.ManifestSourceRemote.Human()),
" ",
fmt.Sprintf(style.Highlight(`Your manifest source is "%s"`), manifestSource.String()),
fmt.Sprintf(`App manifest source can be %s or %s:`, config.ManifestSourceLocal.Human(), config.ManifestSourceRemote.Human()),
fmt.Sprintf("- %s: uses manifest from your project's source code for all apps", config.ManifestSourceLocal.String()),
Copy link
Member

@zimeg zimeg May 14, 2025

Choose a reason for hiding this comment

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

Suggested change
fmt.Sprintf("- %s: uses manifest from your project's source code for all apps", config.ManifestSourceLocal.String()),
fmt.Sprintf("- %s: uses manifest from your project's source code on reinstallation", config.ManifestSourceLocal.String()),

📚 This might not be a good suggestion, but confusion could arise with when these updates are.

Perhaps this is alright though! Another command might make this difference more obvious in later iteration?

$ slack manifest update

Copy link
Member

@zimeg zimeg May 14, 2025

Choose a reason for hiding this comment

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

Edit: This comment makes me think "on update" is better wording, but again no blocker!

Suggested change
fmt.Sprintf("- %s: uses manifest from your project's source code for all apps", config.ManifestSourceLocal.String()),
fmt.Sprintf("- %s: uses manifest from your project's source code on update", config.ManifestSourceLocal.String()),

Copy link
Member Author

@mwbrooks mwbrooks May 14, 2025

Choose a reason for hiding this comment

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

I agree that this can be confusing. Unfortunately, the project-level manifest is used in many places and not just updates/re-installs. For example, slack manifest validate but also many areas of the internal-workings such as checking whether the app is hosted or not (a pre-run for some commands).

Looking ahead, this warning is probably going to be removed or reduced to a single line such as:

🤖 Some Title
   Reading app manifest from app settings
   Reading app manifest from project

So, we probably don't need to overthink the current warning too much.

Copy link
Member

@zimeg zimeg May 15, 2025

Choose a reason for hiding this comment

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

@mwbrooks 🧠 ✨ Of course, how could I forget!

Unfortunately, the project-level manifest is used in many places

I will celebrate this learning with good a fortune 👾

fmt.Sprintf("- %s: uses manifest from app settings for each app", config.ManifestSourceRemote.String()),
" ",
fmt.Sprintf(style.Highlight(`Your manifest source is set to %s.`), manifestSource.Human()),
" ",
fmt.Sprintf("Current manifest source in %s:", style.Highlight(filepath.Join(config.ProjectConfigDirName, config.ProjectConfigJSONFilename))),
fmt.Sprintf(style.Highlight(` %s: "%s"`), "manifest.source", manifestSource.String()),
" ",
Expand Down
2 changes: 1 addition & 1 deletion cmd/project/init_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ func Test_Project_InitCommand(t *testing.T) {
require.Contains(t, output, "Added "+filepath.Join("project-name", ".slack"))
require.Contains(t, output, "Added "+filepath.Join("project-name", ".slack", ".gitignore"))
require.Contains(t, output, "Added "+filepath.Join("project-name", ".slack", "hooks.json"))
require.Contains(t, output, "Updated config.json manifest source to local")
require.Contains(t, output, `Updated config.json manifest source to "project" (local)`)
// Assert prompt to add existing apps was called
cm.IO.AssertCalled(
t,
Expand Down
13 changes: 13 additions & 0 deletions internal/config/manifest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@

package config

import "fmt"

type ManifestSource string

const (
Expand All @@ -35,6 +37,17 @@ func (ms ManifestSource) String() string {
return string(ms)
}

// Human returns the string value as a human-friendly name
func (ms ManifestSource) Human() string {
switch ms {
case ManifestSourceLocal:
return fmt.Sprintf(`"project" (%s)`, ms.String())
case ManifestSourceRemote:
return fmt.Sprintf(`"app settings" (%s)`, ms.String())
}
return ms.String()
}

type ManifestConfig struct {
// Source of the manifest using either "local" or "remote" values
Source string `json:"source,omitempty"`
Expand Down
30 changes: 28 additions & 2 deletions internal/config/manifest_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,11 @@ func Test_Config_ManifestSource_String(t *testing.T) {
a ManifestSource
expected string
}{
"project manifest source is local": {
"local manifest source": {
a: ManifestSourceLocal,
expected: "local",
},
"remote manifest source is remote": {
Copy link
Member

@zimeg zimeg May 14, 2025

Choose a reason for hiding this comment

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

🪓 🪓

"remote manifest source": {
a: ManifestSourceRemote,
expected: "remote",
},
Expand All @@ -100,3 +100,29 @@ func Test_Config_ManifestSource_String(t *testing.T) {
})
}
}

func Test_Config_ManifestSource_Human(t *testing.T) {
tests := map[string]struct {
a ManifestSource
expected string
}{
"local manifest source is the project (local)": {
a: ManifestSourceLocal,
expected: `"project" (local)`,
},
"remote manifest source is app settings (remote)": {
a: ManifestSourceRemote,
expected: `"app settings" (remote)`,
},
"unknown manifest source uses String()": {
a: "unknown",
expected: "unknown",
},
}
for name, tt := range tests {
t.Run(name, func(t *testing.T) {
actual := tt.a.Human()
assert.Equal(t, tt.expected, actual)
})
}
}
2 changes: 1 addition & 1 deletion internal/pkg/create/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,7 +466,7 @@ func InstallProjectDependencies(
clients.IO.PrintDebug(ctx, "Error setting manifest source in project-level config: %s", err)
} else {
configJSONFilename := config.ProjectConfigJSONFilename
manifestSourceStyled := style.Highlight(manifestSource.String())
manifestSourceStyled := style.Highlight(manifestSource.Human())

outputs = append(outputs, fmt.Sprintf(
"Updated %s manifest source to %s",
Expand Down
6 changes: 3 additions & 3 deletions internal/pkg/create/create_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,17 +195,17 @@ func Test_Create_installProjectDependencies(t *testing.T) {
"Detected a project using Deno",
},
},
"When no manifest source, default to local": {
"When no manifest source, default to project (local)": {
experiments: []string{"bolt"},
expectedOutputs: []string{
"Updated config.json manifest source to local",
`Updated config.json manifest source to "project" (local)`,
},
},
"When manifest source is provided, should set it": {
experiments: []string{"bolt"},
manifestSource: config.ManifestSourceRemote,
expectedOutputs: []string{
"Updated config.json manifest source to remote",
`Updated config.json manifest source to "app settings" (remote)`,
},
},
}
Expand Down
Loading