Skip to content

Conversation

@mwbrooks
Copy link
Member

@mwbrooks mwbrooks commented Apr 1, 2025

Summary

This pull request updates the URL in the hooks.json error message to use a working, valid URL.

While the Slack CLI now support .slack/hooks.json, it continues to remain backwards compatible with slack.json. We're in a bit of a messy-in-the-middle situation where the Slack CLI often references .slack/hooks.json but our samples continue to use slack.json for backwards compatibility.

I've updated the error messages to also suggest using init to create the file, when it's missing.

Preview

Error when a hook script returns an error code:
image

Error when the hooks file is missing:
image

Reviewer

# Create a new Deno SDK app
$ lack create my-app --template https://github.com/slack-samples/deno-starter-template
$ cd my-app/

# Error when a hook script returns an error code:
$ vim slack.json
# → Update "deno" to "dino", or some other binary name that doesn't exist
$ lack run
# → Expect an error message similar to the preview screenshot

# Error when the hooks file is missing:
$ mv slack.json slack.json.bk
$ lack run
# → Expect an error message similar to the preview screenshot

# Cleanup
$ lack delete -f
$ cd ../
$ rm -rf my-app/

Requirements

@mwbrooks mwbrooks added 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 labels Apr 1, 2025
@mwbrooks mwbrooks added this to the Next Release milestone Apr 1, 2025
@mwbrooks mwbrooks self-assigned this Apr 1, 2025
@codecov
Copy link

codecov bot commented Apr 1, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.80%. Comparing base (bb9a1f7) to head (d612004).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main       #3      +/-   ##
==========================================
- Coverage   62.82%   62.80%   -0.02%     
==========================================
  Files         210      210              
  Lines       22053    22053              
==========================================
- Hits        13854    13851       -3     
- Misses       7124     7126       +2     
- Partials     1075     1076       +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.

@mwbrooks mwbrooks marked this pull request as ready for review April 1, 2025 18:30
@mwbrooks mwbrooks requested a review from a team as a code owner April 1, 2025 18:30
Copy link
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.

LGTM! ✨

I've been thinking so much about supporting prior CLI versions expecting slack.json in past CLI versions and I unfortunately don't think we have a migration possible for those versions.

Revisiting sample versioning might be interesting, perhaps bringing tags to the create command, but that's all idea.

This PR is a nice improvement for these cases as is!

I left comments on wording, but more around spacing in linebreaks. I'm so interested in what you think. I realize dense outputs can be offputting, but I'm hoping it becomes familiar in such common errors 👾

Comment on lines 1299 to 1301
"Hook scripts are defined in a Slack hooks file:",
"- slack.json",
"- or",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"Hook scripts are defined in a Slack hooks file:",
"- slack.json",
"- or",
"Hook scripts are defined in a Slack hooks file found at either:",
"- slack.json",

🤔 Hmm I think the list of items makes the "or" somewhat confusing for me.

I'm not sure if this is great wording, but I'm hoping to make the "pick one" option more clear in the line above!

Copy link
Member Author

Choose a reason for hiding this comment

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

That's definitely a fair suggestion. I wasn't a fan of the "or" but I wanted to keep it readable.

Riffing on your suggestion, commit b9588ea updates the text to end with "Slack hooks files" but tries to emphasize that it can be one of the following:

   Hook scripts are defined in one of these Slack hooks files:
   - slack.json
   - .slack/hooks.json

"Hook scripts are defined in a Slack hooks file:",
"- slack.json",
"- or",
"- " + filepath.Join(".slack", "hooks.json"),
Copy link
Member

Choose a reason for hiding this comment

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

🎉 Love the filepath support here!

"",
style.Highlight("https://github.com/slack-samples/deno-starter-template/blob/main/.slack/hooks.json"),
"You can create a hooks file manually or with " + style.Commandf("init", false),
"",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"",

Nit: IMO newlines between some of these details separates some related ideas, but these might be helpful for parsing outputs... 👁️‍🗨️

No blocker but removing some might be interesting!

Copy link
Member Author

Choose a reason for hiding this comment

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

I hear ya on too much separation, but I think it's important to leave a newline after a URL when possible. Depending on the person's terminal, the URL may not be styled and may wrap to multiple lines. So, leaving some whitespace after the URL can help it stand out and make the entire sentence more readable.

For example, here is a smaller window size. I've merged the previous suggestion to remove the whitespace before the URL. However, if we removed the whitespace after, it will become very unreadable:
image

Comment on lines +813 to +814
"",
"If this is a Slack project, you can initialize it with " + style.Commandf("init", false),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"",
"If this is a Slack project, you can initialize it with " + style.Commandf("init", false),
"You can initialize an existing Slack project with the " + style.Commandf("init", false) + " command",

I like the wording to the reader! I made a similar line removal here with some adjusted words, but might want to discuss the extra line more. Without having a similar suggestion heading this feels somewhat out of place to me...

Not meaning to block this from merging but am also hoping to lean into fewer linebreaks in output overall? It's at least interesting to me.

@mwbrooks
Copy link
Member Author

mwbrooks commented Apr 1, 2025

Thanks for the ideas @zimeg! Here's the final result of bringing everything in!

image

🧠 While iterating on this error, it makes me think that we should try to become more crisp on whether to have complete sentences with a . period or not.

@mwbrooks mwbrooks merged commit 0f8c092 into main Apr 1, 2025
6 checks passed
@mwbrooks mwbrooks deleted the mbrooks-slack-json-error-url branch April 1, 2025 20:55
@zimeg
Copy link
Member

zimeg commented Apr 2, 2025

💌 Once this releases it might be nice to follow up with the kind note of slack-samples/deno-starter-template#76!

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.

3 participants