refactor: remove 'version.Get()' and only use 'version.Raw()' by mwbrooks · Pull Request #445 · slackapi/slack-cli

mwbrooks · 2026-03-27T20:59:39Z

Changelog

N/A

Summary

This pull request removes version.Get() and only uses version.Raw(). The motivation is the simplify version handling to be 1 method instead of 2 methods.

Context:

PR #429 consolidated version handling to prefer version.Raw() and slackcontext.Version(ctx). A follow-up task was to remove version.Get() entirely since it creates confusion about which function to use. The only difference is that .Get() prepends a v prefix if missing - but we can guarantee the v prefix is always present without requiring .Get().

Why it's Safe to Remove:

The v prefix is already guaranteed in all code paths that set Version:

Build-Time (Makefile): git describe --tags --match 'v*.*.*' - the --match pattern requires a v-prefixed tag, and git describe outputs the matching tag name, so the result always starts with v.
Default Value (version.go): "v0.0.0-dev" - already has the prefix.
Env Var Override (version.go): SLACK_TEST_VERSION - this is the one gap. QA testers could set it without a v. This PR adds validation by moving the v-prefix enforcement from .Get() into init(), ensuring Version is normalized at startup.

Open Question 🧠

Do we want to rename version.Raw() to version.Version()?
- Technically, it's no longer the "raw" version because it may have a v-prefix added.
- In the future, we may want a version.Major(), version.Minor(), etc to get specific elements of the version.

Requirements

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.

Move the v-prefix enforcement from Get() into init(), ensuring Version is normalized at startup. This eliminates the confusing Get() vs Raw() distinction — Raw() is now the single way to access the version.

codecov · 2026-03-27T21:02:53Z

Codecov Report

❌ Patch coverage is 53.84615% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.33%. Comparing base (7514e97) to head (bc1d164).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/pkg/auth/login.go	0.00%	3 Missing ⚠️
cmd/root.go	33.33%	2 Missing ⚠️
cmd/upgrade/upgrade.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #445      +/-   ##
==========================================
- Coverage   70.34%   70.33%   -0.01%     
==========================================
  Files         220      220              
  Lines       18506    18506              
==========================================
- Hits        13018    13017       -1     
- Misses       4309     4312       +3     
+ Partials     1179     1177       -2

☔ 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

A few notes for the semantic minded versioners 🔢

mwbrooks · 2026-03-27T21:01:02Z

 	if envVersion := getVersionFromEnv(); envVersion != "" {
 		Version = envVersion
 	}
+	Version = ensurePrefix(Version)


note: We now enforce the v prefix on init() so that all version-pathways will now have a v prefix.

🦠 thought: I like this approach! It keeps the ctx corrent and commands are well structured for this I believe!

zimeg

@mwbrooks The version package gets good updates 🎁 ✨

I'm approving with a few suggestions and no blockers 🚢 I'm most curious about using slackcontext.Version as much as possible to keep the version package imports few?

Do we want to rename version.Raw() to version.Version()?

This is a solid change also and matches what I'd expect! I like the follow up to semantic version parsings 🤓

zimeg · 2026-03-30T04:30:04Z

 	ctx := cmd.Context()
-	updateNotification := update.New(clients, version.Get(), "SLACK_SKIP_UPDATE")
+	updateNotification := update.New(clients, version.Raw(), "SLACK_SKIP_UPDATE")


🎁 suggestion(non-blocking): In later changes to upgrade do we want to prefer slackcontext packages instead of importing version here?

slack-cli/internal/slackcontext/slackcontext.go

Lines 174 to 184 in e114959

// Version returns the CLI version associated with `ctx`. If the version is not

// found in the context, it falls back to version.Raw() and returns an error to

// signal that the context was not properly initialized.

func Version(ctx context.Context) (string, error) {

var v, ok = ctx.Value(contextKeyVersion).(string)

if !ok || v == "" {

return version.Raw(), slackerror.New(slackerror.ErrContextValueNotFound).

WithMessage("The value for version could not be found in context, falling back to the build version")

}

return v, nil

}

Good suggestion. During this PR, I was surprised to see so many version.Raw() calls still remain. In the follow-up PR, I'll aim to replace these - it's less dangerous now that slackcontext.Version(ctx) fallsback on the raw version. 👍🏻

zimeg · 2026-03-30T04:33:39Z

-		version = "v" + version
+// ensurePrefix ensures that the version string has a "v" prefix.
+func ensurePrefix(v string) string {
+	if match, _ := regexp.MatchString(`^[^v]`, v); match {


Suggested change

if match, _ := regexp.MatchString(`^[^v]`, v); match {

if !strings.HasPrefix(v, "v") {

🌚 suggestion: While we're improving this we might import from string instead?

📚 https://pkg.go.dev/strings#HasPrefix

Good suggestion! Thanks for keeping an eye out for opportunities to reduce our regex-foo!

Commit 218fa43 replaces regexp.MatchString with strings.HasPrefix. The ensurePrefix method must also now handle the empty string use-case, which our unit tests already cover.

zimeg · 2026-03-30T04:34:36Z

-			original := Version
-			defer func() { Version = original }()
-			Version = tc.version
-			assert.Equal(t, tc.expected, Get())


🪓 praise: Huge improvement IMHO!

zimeg · 2026-03-30T04:35:57Z

 	if envVersion := getVersionFromEnv(); envVersion != "" {
 		Version = envVersion
 	}
+	Version = ensurePrefix(Version)


🦠 thought: I like this approach! It keeps the ctx corrent and commands are well structured for this I believe!

mwbrooks · 2026-03-30T16:36:06Z

Thanks for the review @zimeg! Thorough as always the PR is walking away better because of your thoughts 🧠

I've addressed your suggest to swap the regex for strings 🥷 And I have 2 follow-up PRs for the other suggestions:

Replace version.Raw() with version.Version().
Replace more version.Raw() with slackcontext.Version(ctx) - more risky, but the risk should be lowered now that we fallback on version.Raw().

refactor: remove version.Get() and use only version.Raw()

e114959

Move the v-prefix enforcement from Get() into init(), ensuring Version is normalized at startup. This eliminates the confusing Get() vs Raw() distinction — Raw() is now the single way to access the version.

mwbrooks added this to the Next Release milestone Mar 27, 2026

mwbrooks self-assigned this Mar 27, 2026

mwbrooks added code health M-T: Test improvements and anything that improves code health semver:patch Use on pull requests to describe the release version increment labels Mar 27, 2026

mwbrooks commented Mar 27, 2026

View reviewed changes

mwbrooks marked this pull request as ready for review March 27, 2026 21:05

mwbrooks requested a review from a team as a code owner March 27, 2026 21:05

zimeg approved these changes Mar 30, 2026

View reviewed changes

mwbrooks added 2 commits March 30, 2026 09:26

refactor: use strings.HasPrefix instead of regexp in ensurePrefix

218fa43

Merge branch 'main' into mwbrooks-remove-version-get

bc1d164

mwbrooks merged commit 922813e into main Mar 30, 2026
8 checks passed

mwbrooks deleted the mwbrooks-remove-version-get branch March 30, 2026 16:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: remove 'version.Get()' and only use 'version.Raw()'#445

refactor: remove 'version.Get()' and only use 'version.Raw()'#445
mwbrooks merged 3 commits intomainfrom
mwbrooks-remove-version-get

mwbrooks commented Mar 27, 2026 •

edited

Loading

Uh oh!

codecov Bot commented Mar 27, 2026 •

edited

Loading

Uh oh!

mwbrooks left a comment

Uh oh!

mwbrooks Mar 27, 2026

Uh oh!

zimeg Mar 30, 2026

Uh oh!

zimeg left a comment

Uh oh!

zimeg Mar 30, 2026

Uh oh!

mwbrooks Mar 30, 2026

Uh oh!

zimeg Mar 30, 2026

Uh oh!

mwbrooks Mar 30, 2026

Uh oh!

zimeg Mar 30, 2026

Uh oh!

zimeg Mar 30, 2026

Uh oh!

mwbrooks commented Mar 30, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	// Version returns the CLI version associated with `ctx`. If the version is not
	// found in the context, it falls back to version.Raw() and returns an error to
	// signal that the context was not properly initialized.
	func Version(ctx context.Context) (string, error) {
	var v, ok = ctx.Value(contextKeyVersion).(string)
	if !ok \|\| v == "" {
	return version.Raw(), slackerror.New(slackerror.ErrContextValueNotFound).
	WithMessage("The value for version could not be found in context, falling back to the build version")
	}
	return v, nil
	}

	if match, _ := regexp.MatchString(`^[^v]`, v); match {
	if !strings.HasPrefix(v, "v") {

Conversation

mwbrooks commented Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog

Summary

Open Question 🧠

Requirements

Uh oh!

codecov Bot commented Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mwbrooks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zimeg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mwbrooks commented Mar 30, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mwbrooks commented Mar 27, 2026 •

edited

Loading

codecov Bot commented Mar 27, 2026 •

edited

Loading