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 2 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",
fmt.Sprintf("managed by %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()),
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.

🗣️ Another comment suggests removing the quotations with Human but I'd be curious how this reads.

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 think for this specific line, we want to tell the developer what their current manifest source is set to. I stuck with the .String() but we could switch it to the .Human().

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 think for now, let's leave it as-is. We already list the options above this text. Originally, the text was meant to explain what the project's current configuration is. Underneath, we then show the value in config.json and explain what the value will be changed to.
image

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 "%s"`), manifestSource.Human()),
" ",
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.

note: I tweaked the wording now that we've had some time to reflect on this. Once we default Bolt projects to remote manifest, this warning will no longer be shown, so it's not a long-lived change.

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.

🙏 ✨ This section has been confusing to me before, but I hadn't realized that a defaulted remote manifest source will avoid this altogether!

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: For my own understanding, will it still be shown but instead much less often?

I can imagine changing the manifest source to local for automated productions then linking an app later, though this might not be the common case...

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.

Good question! I think the warning becomes less common after bolt-install and we could consider changing it to a message instead of a prompt.

The warning should be displayed when a project manages the app manifest (manifest.source: "local") and an existing app is linked (slack app link).

Right now, the warning prompts you to switch to a remote manifest source. I think we have an opportunity to make this more flexible. There are some cases where you may want to link an app while keeping your local manifest source.

Long story short, we'll want to think more about this as we make decisions around bolt-install 😆

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())
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.

🤔 This might be confusing, but I have a small favor towards quoting "project" while keeping (local) and (remote) in parentheses.

To me it'd read better in lines like:

App manifest source can be "project" (local) or "app settings" (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.

Edit: But perhaps we remove quotes in these lines altogether? Or handle the cases of "project" and "app settings" in configuration, but with warning to use the preferred local or remote?

I now lean towards removing quotes, but I'd love to know what is most clear to you!

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.

Great suggestion. I think the "project" (local) format is an improvement! I'll update the tests and commit the changes! 🙇🏻

image

image

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.

Commit 4eaea06 now quotes the human-friendly name and wraps the ID in parentheses.

case ManifestSourceRemote:
return fmt.Sprintf("app settings (%s)", ms.String())
}
return ""
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
return ""
return ms.String()

Ought not we find a confusing blank output here in some strange case 😉

Edit: A test for this too would be superb but is also no blocker for me 🤖 ✨

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.

Great eye! Commit 3913b13 handles unknown manifest sources with your suggestion above and adds a test case 👌🏻

}

type ManifestConfig struct {
// Source of the manifest using either "local" or "remote" values
Source string `json:"source,omitempty"`
Expand Down
26 changes: 24 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,25 @@ 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)",
},
}
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