fix: format env command section headers together as titles#461
Conversation
zimeg
added
bug
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #461 +/- ##
=======================================
Coverage 71.12% 71.12%
=======================================
Files 220 220
Lines 18547 18530 -17
=======================================
- Hits 13192 13180 -12
+ Misses 4178 4175 -3
+ Partials 1177 1175 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
📋 note: This follows feedback from #451! |
mwbrooks
left a comment
mwbrooks
left a comment
There was a problem hiding this comment.
✅ Thank you so much for standardizing the headers! I've left a question that I'll detail below, but approaching to keep this PR unblocked. 🎉
💬 I've left a question around whether we want to change this to Set/Unset based on PR #460. Either way, I'll approve this PR so that it's not blocked by nits because it's an improvement either way.
cmd/env/set.go
Outdated
| fmt.Sprintf("Successfully added \"%s\" as an app environment variable", variableName), | ||
| }, | ||
| })) | ||
| details = append(details, fmt.Sprintf("Successfully added \"%s\" as an app environment variable", variableName)) |
There was a problem hiding this comment.
question: Should we start using the terms "set" and "unset" based on PR #460? While using another word like "added" might solidify what's happening, I think "set" is more accurate because it may be adding or updating the key.
There was a problem hiding this comment.
@mwbrooks @srtaalej Noted in #460 also that "remove" might have a few places to change! I agree with both comments and mirror this here to not forget 👾
looking great!! i noticed there are still a few references to removing in the file 👁️🗨️
line 120 "Select a variable to remove"
line 150 "Successfully removed "%s" from the app's environment variables"
might be nice to update these too 🚀
There was a problem hiding this comment.
🧪 note: This will require updates to our E2E tests too!
cmd/env/add.go
Outdated
| clients.IO.PrintTrace(ctx, slacktrace.EnvAddSuccess) | ||
| clients.IO.PrintInfo(ctx, false, "\n%s", style.Sectionf(style.TextSection{ | ||
| Emoji: "evergreen_tree", | ||
| Text: "Environment Add", |
There was a problem hiding this comment.
question: Based on #460 what about?
- Set Environment Variable
- Unset Environment Variable
- List Environment Variables (plural)
| Text: "Environment Add", | |
| Text: "Set Environment Variable", |
There was a problem hiding this comment.
@mwbrooks Haha I realize these branches overlap! Would we want to use:
- Environment Set
- Environment List
- Environment Unset
Instead? I agree this reads odd but matching the command name might let use expand more in details that follow I hope 📚
srtaalej
left a comment
srtaalej
left a comment
There was a problem hiding this comment.
looks good! it'd be nice to use set and unset everywhere but non-blocking 😁 thank you for these very nice changes to env!
Accept main's rename of add→set and remove→unset while preserving the branch's section header consolidation pattern across all env commands. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Co-authored-by: Ale Mercado <maria.mercado@slack-corp.com> Co-authored-by: Michael Brooks <mbrooks@slack-corp.com>
3 participants
Changelog
Summary
This PR formats the
envcommand section headers together in a standard title format.Preview
Notes
env removecommand supports the ".env" file for Bolt Framework apps #457 and feat: aliasenv setandenv unsetas primary commands #460 with changes to aliases and command features which might make review odd. I hope to keep these branches updated with the latest changes onmainthough!Requirements