refactor: enables linter rule ST1003 'poorly chosen identifier' by mwbrooks · Pull Request #69 · slackapi/slack-cli

mwbrooks · 2025-04-28T17:29:11Z

Summary

This pull request enables the Linter Rule ST1003 "Poorly Chosen Identifier" and updates all identifiers to match the new rule.

For the most part, this changing Id → ID, Http → HTTP, SOME_CONSTANT → SomeConstant, etc.

There is no change to functionality. 🚳

Reviewers

I'm so so so so sorry for the HUGE PR. I think this will be a common theme as we enable more of the default linter rules. 😆

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.

codecov · 2025-04-28T17:31:53Z

Codecov Report

Attention: Patch coverage is 59.06623% with 377 lines in your changes missing coverage. Please review.

Project coverage is 63.00%. Comparing base (5d08f1c) to head (1f6967e).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
cmd/triggers/access.go	50.00%	34 Missing and 8 partials ⚠️
internal/pkg/platform/activity.go	17.02%	39 Missing ⚠️
internal/api/app.go	50.00%	34 Missing ⚠️
internal/api/collaborators.go	0.00%	20 Missing ⚠️
internal/api/datastore.go	55.55%	16 Missing ⚠️
internal/pkg/auth/login.go	0.00%	13 Missing ⚠️
internal/auth/auth.go	57.14%	9 Missing and 3 partials ⚠️
internal/api/functiondistribution.go	50.00%	10 Missing and 1 partial ⚠️
internal/api/workflows.go	47.36%	10 Missing ⚠️
internal/config/context.go	0.00%	10 Missing ⚠️
... and 45 more

Additional details and impacted files

@@           Coverage Diff           @@
##             main      #69   +/-   ##
=======================================
  Coverage   63.00%   63.00%           
=======================================
  Files         210      210           
  Lines       22186    22186           
=======================================
  Hits        13978    13978           
  Misses       7116     7116           
  Partials     1092     1092

☔ 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

I'm super sorry for the long read! Opening this PR in my browser brings my computer to a crawl. 🐜 There is no change to functionality, but I've made a few notes on a few minor naming decisions that I made.

mwbrooks · 2025-04-28T17:29:50Z

    staticcheck:
      checks:
        - all
-        - '-ST1003' # disable rule 'Poorly chosen identifier'


note: This is the main change. I enabled this rule and then updates all linting errors.

mwbrooks · 2025-04-28T17:30:21Z

 	if err != nil {
 		return err
 	}
-	if manifestSource.Equals(config.MANIFEST_SOURCE_REMOTE) {


note: All capital constants are not allowed.

This is a sensible rule IMO! While it's not as clear that these are constants at a glance, I do believe it'll encourage better patterns overall.

mwbrooks · 2025-04-28T17:31:45Z

 				appSelectMock.On("TeamAppSelectPrompt").Return(prompts.SelectedApp{Auth: mockAuthTeam1}, nil)

 				// Mock valid session for team1
-				cm.ApiInterface.On("ValidateSession", mock.Anything, mock.Anything).Return(api.AuthSession{


note: acronyms should be capitalized - this is where most of the changes occurred because we consistently stuck with Api, Http, etc.

I am so glad for this change. No longer will I have questions about these casings 👾

mwbrooks · 2025-04-28T17:32:42Z

 							User     string "json:\"user,omitempty\""
 						}{},
 					},
-					types.SUCCESS,


note: Since types.SUCCESS has no indication that it's a install success, I've prefixed all InstallState types to be InstallSuccess. I did the same for Permissions.

While outside the scope of this PR, this makes me think certain types might be better suited for different packages.

The prefix makes it clear for now though!

100% agree. I'd like to see the shared/types package removed entirely. Instead, the types/interfaces/etc should belong to the package they're intended for. It would also allow for self-documentation, such as install.Success.

zimeg

@mwbrooks These are wonderful improvements to the health of this codebase. It is so good to find the oddities a linter might catch!

I requested changes on this with a few notes. The first is important IMO, but the others I think are worth addressing before we can merge this:

Within samples we should not change the HTTP headers - reference
A few instances of Cli were found, but CLI to me is more correct
An instance of "CallBack" might be changed to "Callback" AFAICT
A similar note on "metaData" versus "metadata" might be worth changing

All superb efforts on these changes, I can tell, but please let me know what you think or these suggestions. I am of course open to discussion 🙏 ✨

zimeg · 2025-04-29T17:54:56Z

 	if err != nil {
 		return err
 	}
-	if manifestSource.Equals(config.MANIFEST_SOURCE_REMOTE) {


This is a sensible rule IMO! While it's not as clear that these are constants at a glance, I do believe it'll encourage better patterns overall.

zimeg · 2025-04-29T17:56:05Z

 				appSelectMock.On("TeamAppSelectPrompt").Return(prompts.SelectedApp{Auth: mockAuthTeam1}, nil)

 				// Mock valid session for team1
-				cm.ApiInterface.On("ValidateSession", mock.Anything, mock.Anything).Return(api.AuthSession{


I am so glad for this change. No longer will I have questions about these casings 👾

zimeg · 2025-04-29T17:58:08Z

 							User     string "json:\"user,omitempty\""
 						}{},
 					},
-					types.SUCCESS,


While outside the scope of this PR, this makes me think certain types might be better suited for different packages.

The prefix makes it clear for now though!

zimeg · 2025-04-29T17:59:47Z

 				}, nil)
 				// Mock delete API call
-				cm.ApiInterface.On("DeleteApp", mock.Anything, mock.Anything, fakeDeployedApp.AppID).Return(fmt.Errorf("something went terribly wrong"))
+				cm.APIInterface.On("DeleteApp", mock.Anything, mock.Anything, fakeDeployedApp.AppID).Return(fmt.Errorf("something went terribly wrong"))


I'm super curious if a later linter change might check specific formatting of errors 🔍

Yes! ST1005 checks the formatting of an error. It requires the first letter to be lowercase, unless it's a proper Noun (e.g. Slack) and it cannot end with a period. The reason is that errors in Golang are often fmt.Sprintf("HTTP Request %s return an error: %s", req.URL, err) embedded in a message.

So far, I don't see too much emphasize on whether we use fmt.Errorf or other error methods.

zimeg · 2025-04-29T18:31:18Z


 	// Mock API calls
-	clientsMock.AuthInterface.On("ResolveApiHost", mock.Anything, mock.Anything, mock.Anything).
+	clientsMock.AuthInterface.On("ResolveAPIHost", mock.Anything, mock.Anything, mock.Anything).


String replacements seem like quite a tedious change! 🧵

zimeg · 2025-04-29T19:30:20Z

-var SIXTY_FOUR_KB_STRING = string(make([]byte, (64*1024)+1))
-var FIVE_HUNDRED_TWELVE_KB_STRING = string(make([]byte, (512*1024)+1))
+var mockBoundaryString = "boundary-string"
+var sixtyFourKBString = string(make([]byte, (64*1024)+1))


🎶 🐞 ✨

zimeg · 2025-04-29T19:37:09Z

 	}
 	req.Header.Set("Accept", "application/vnd.github+json")
-	req.Header.Set("X-GitHub-Api-Version", "2022-11-28")
+	req.Header.Set("X-GitHub-API-Version", "2022-11-28")


Suggested change

req.Header.Set("X-GitHub-API-Version", "2022-11-28")

req.Header.Set("X-GitHub-Api-Version", "2022-11-28")

We might not want to make this change without an upstream fix 🐙

An option to exclude this entire string might be an option, but I am not certain...

https://docs.github.com/en/rest/about-the-rest-api/api-versions?apiVersion=2022-11-28

Incredible catch @zimeg ❤️ Thank you so much - this is why we have PR reviewers 🙇🏻

Commit 485109b fixes this reference in 2 places

zimeg · 2025-04-29T19:45:00Z

 	// Internal dependencies

-	ApiInterface func() api.ApiInterface
+	APIInterface func() api.APIInterface


Since we now have just the interface implementation attached to clients I am almost wanting to rename this to API 😳

Of course, this would be a change better for another PR but I am also so curious about what you might think?

Yes, please. I cringe every time I see a method called on an interface. I'll make a note for a follow-up PR. 📝

zimeg · 2025-04-29T19:51:44Z

 )

-const metaDataUrl = "https://api.slack.com/slackcli/metadata.json"
+const metaDataURL = "https://api.slack.com/slackcli/metadata.json"


Suggested change

const metaDataURL = "https://api.slack.com/slackcli/metadata.json"

const metadataURL = "https://api.slack.com/slackcli/metadata.json"

Without referencing this exact comment, I believe we can lowercase most of this 🥀

Great eyes! PR #71 updates metaData → metadata and MetaData → Metadata for easier review! 🌹

zimeg · 2025-04-29T19:55:32Z

 // Test_CLI_Metadata_CheckForUpdate tests different responses from Slack CLI metadata.
 func Test_CLI_Metadata_CheckForUpdate(t *testing.T) {
-	const metaDataUrl = "https://api.slack.com/slackcli/metadata.json"
+	const metaDataURL = "https://api.slack.com/slackcli/metadata.json"


Suggested change

const metaDataURL = "https://api.slack.com/slackcli/metadata.json"

const metadataURL = "https://api.slack.com/slackcli/metadata.json"

Same change in a different scope 🦠

mwbrooks · 2025-04-30T03:23:51Z

@zimeg Wow, I can't believe you carefully read through this entire PR! 🫨 Thank you for the thorough review, because the request header for X-GitHub-Api-Version may have introduced a bug! 🐛 🙇🏻 🥂 Commit 485109b fixes the issue 🙌🏻

I've created PR #71 to address your other suggestions, because the changes can get lost in this PR. I've left it as a draft until we merge this one, but it's ready to be opened! 😆

zimeg

@mwbrooks Heh it was a good excuse to read a lot of code 🤓

I'm glad these efforts might've paid off too! I find the browser UI to be unforgiving on these diff and stumbled upon an exciting tool called delta that deserves most of the credit:

At a glance, the changes of #71 are great follow ups! I agree that these are additional improvements that deserve separate comment. Thanks for keeping things tidy 📚

And this is also such an exciting PR to merge! I don't recall changes of this magnitude in recent commits, but these changes are still somehow so focused 🧠 ✨

mwbrooks · 2025-04-30T16:33:20Z

Woo woo! Thanks for the final approval @zimeg! 🍎 I'm still super impressed that you found the GitHub Header issue :amaze:

delta looks like a serious tool and I appreciate that the maintainer includes the git configuration! I'll have to try it out and upgrade from my default diff. The main advantage to me is highlighting the exact character changes! 🟩 🟩 🟩

Once merged, I'll open PR #71 and I've left a few comments on some open questions for us to decide! 🙇🏻

mwbrooks added 4 commits April 27, 2025 20:23

refactor: update names to match ST1003 lint rule

410f2d5

refactor: update Http to HTTP for ST1003 lint rule

df74aec

refactor: ST1003 linter updates

f66f209

refactor: ST1003 linter updates

834eecf

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 Apr 28, 2025

mwbrooks added this to the Next Release milestone Apr 28, 2025

mwbrooks self-assigned this Apr 28, 2025

mwbrooks commented Apr 28, 2025

View reviewed changes

mwbrooks marked this pull request as ready for review April 28, 2025 17:34

mwbrooks requested a review from a team as a code owner April 28, 2025 17:34

zimeg requested changes Apr 29, 2025

View reviewed changes

mwbrooks mentioned this pull request Apr 30, 2025

refactor: update custom initialisms for linter rule ST1003 #71

Merged

2 tasks

mwbrooks added 2 commits April 29, 2025 19:38

refactor: fix GitHub header to match required 'Api' case

485109b

Merge branch 'main' into mwbrooks-lint-st1003

1f6967e

mwbrooks requested a review from zimeg April 30, 2025 03:24

zimeg approved these changes Apr 30, 2025

View reviewed changes

mwbrooks merged commit ec644b2 into main Apr 30, 2025
6 checks passed

mwbrooks deleted the mwbrooks-lint-st1003 branch April 30, 2025 16:33

	req.Header.Set("X-GitHub-API-Version", "2022-11-28")
	req.Header.Set("X-GitHub-Api-Version", "2022-11-28")

	const metaDataURL = "https://api.slack.com/slackcli/metadata.json"
	const metadataURL = "https://api.slack.com/slackcli/metadata.json"

Conversation

mwbrooks commented Apr 28, 2025

Summary

Reviewers

Requirements

Uh oh!

codecov Bot commented Apr 28, 2025 • 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!

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!

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!

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 Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mwbrooks commented Apr 30, 2025

Uh oh!

zimeg left a comment

Choose a reason for hiding this comment

Uh oh!

mwbrooks commented Apr 30, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Apr 28, 2025 •

edited

Loading

mwbrooks Apr 30, 2025 •

edited

Loading