Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 0 additions & 1 deletion .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ linters:
- '-QF1004' # disable rule 'Use strings.ReplaceAll instead of strings.Replace'
- '-QF1008' # disable rule 'Omit embedded fields from selector'
- '-QF1011' # disable rule 'Omit redundant type from variable declaration'
- '-ST1016' # disable rule 'Use consistent method receiver names'
Copy link
Member

Choose a reason for hiding this comment

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

🪓 ✨

# https://golangci-lint.run/usage/linters/#staticcheck
# https://staticcheck.dev/docs/configuration/options/#initialisms
initialisms:
Expand Down
8 changes: 4 additions & 4 deletions internal/iostreams/iostreams_mock.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,13 +98,13 @@ func (m *IOStreamsMock) IsTTY() bool {
}

// SetExitCode sets the desired exit code in a thread-safe way
func (io *IOStreamsMock) SetExitCode(code ExitCode) {
atomic.StoreInt32((*int32)(&io.exitCode), int32(code))
func (m *IOStreamsMock) SetExitCode(code ExitCode) {
Copy link
Member

Choose a reason for hiding this comment

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

🧪 Nice! I like mock being m if this is what's found elsewhere!

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, m seems to be a common receiver name for mocks. Makes sense to me and keeps it consistent.

atomic.StoreInt32((*int32)(&m.exitCode), int32(code))
}

// GetExitCode returns the most-recently set desired exit code in a thread safe way
func (io *IOStreamsMock) GetExitCode() ExitCode {
return ExitCode(atomic.LoadInt32((*int32)(&io.exitCode)))
func (m *IOStreamsMock) GetExitCode() ExitCode {
return ExitCode(atomic.LoadInt32((*int32)(&m.exitCode)))
}

// InitLogFile mocks starting the debug info to
Expand Down
24 changes: 12 additions & 12 deletions internal/prompts/app_select.go
Original file line number Diff line number Diff line change
Expand Up @@ -129,20 +129,20 @@ func (apps *TeamApps) IsEmpty() bool {
// Basically, treat as a convenience getter intended for use in the context where
// you have a team you want to filter against and you don't care whether it's an
// auth or an app that corresponds. E.g. when you are comparing to --team flags
func (t *TeamApps) authOrAppTeamDomain() string {
if t.Auth.TeamDomain != "" && (t.Auth.TeamID == t.Hosted.App.TeamID || t.Auth.TeamID == t.Local.App.TeamID) {
func (apps *TeamApps) authOrAppTeamDomain() string {
Copy link
Member Author

Choose a reason for hiding this comment

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

note: The apps receiver name is used the majority of the time. However, I noticed that Golang encourages short, 1-or-2 letter receiver names. In this case a, t, or ta would be more appropriate. However, this file is quite long and apps reads well. So I leaned toward less changes.

Copy link
Member

Choose a reason for hiding this comment

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

In implementations having a more complete variable name seems better to me. Thanks for linking these docs though. It makes for interesting considerations... 🎁

The name need not be as descriptive as that of a method argument, as its role is obvious and serves no documentary purpose.

Sometimes I find it a hassle to search for single letter variables FWIW but this is the most interesting consideration. Perhaps I am needing to read more go 🙏 ✨

Copy link
Member

Choose a reason for hiding this comment

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

Edit: Wow I find the change that follows this in os.go makes sense as a single letter. So much to consider.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, I'm with you. Sometimes the single letter just doesn't feel right. For TeamApps, maybe rethinking the struct's name (or purpose) will help? But, overall I'm cool with apps :)

if apps.Auth.TeamDomain != "" && (apps.Auth.TeamID == apps.Hosted.App.TeamID || apps.Auth.TeamID == apps.Local.App.TeamID) {
// Auth whose team id matches either hosted or local app's team
// Can be safely returned
return t.Auth.TeamDomain
return apps.Auth.TeamDomain
}

// If we get here we might be missing an auth OR the auth doesn't
// match any included app team ids (that is the case when the auth is org
// resolved for a workspace app)
if t.Hosted.App.TeamID != "" {
return t.Hosted.App.TeamDomain
if apps.Hosted.App.TeamID != "" {
return apps.Hosted.App.TeamDomain
}
return t.Local.App.TeamDomain
return apps.Local.App.TeamDomain
}

// authOrAppTeamID greedily returns a team ID corresponding to the TeamApps
Expand All @@ -157,14 +157,14 @@ func (t *TeamApps) authOrAppTeamDomain() string {
// Basically, treat as a convenience getter intended for use in the context where
// you have a team you want to filter against and you don't care whether it's an
// auth or an app that corresponds. E.g. when you are comparing to --team flags
func (t *TeamApps) authOrAppTeamID() string {
if t.Auth.TeamID != "" && (t.Hosted.App.TeamID == t.Auth.TeamID || t.Local.App.TeamID == t.Auth.TeamID) {
return t.Auth.TeamID
func (apps *TeamApps) authOrAppTeamID() string {
if apps.Auth.TeamID != "" && (apps.Hosted.App.TeamID == apps.Auth.TeamID || apps.Local.App.TeamID == apps.Auth.TeamID) {
return apps.Auth.TeamID
}
if t.Hosted.App.TeamID != "" {
return t.Hosted.App.TeamID
if apps.Hosted.App.TeamID != "" {
return apps.Hosted.App.TeamID
}
return t.Local.App.TeamID
return apps.Local.App.TeamID
}

// appTransferDisclaimer contains a notice of lost app management permissions
Expand Down
4 changes: 2 additions & 2 deletions internal/slackdeps/os.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,11 @@ func (c *Os) Glob(pattern string) (matches []string, err error) {
}

// Exit exits the program with a return code
func (m *Os) Exit(code int) {
func (c *Os) Exit(code int) {
os.Exit(code)
}

// Stdout returns the file descriptor for stdout
func (m *Os) Stdout() types.File {
func (c *Os) Stdout() types.File {
return os.Stdout
}
Loading