Skip to content

Conversation

@mwbrooks
Copy link
Member

@mwbrooks mwbrooks commented May 5, 2025

Summary

This pull request follows #77 by updating clients.AuthInterface() to clients.Auth().

The motivation is that the name is shorter and more accurate. The clients.Auth() returns an instance of auth.Client that implements the auth.AuthInterface interface. It doesn't return an interface.

There was no change functionality was changed, but our mocks were updated to match the same naming pattern.

Requirements

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

codecov bot commented May 5, 2025

Codecov Report

Attention: Patch coverage is 82.75862% with 10 lines in your changes missing coverage. Please review.

Project coverage is 63.16%. Comparing base (0526485) to head (8f16237).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
cmd/auth/logout.go 62.50% 0 Missing and 3 partials ⚠️
internal/pkg/auth/login.go 0.00% 2 Missing ⚠️
internal/pkg/apps/list.go 66.66% 1 Missing ⚠️
internal/pkg/platform/deploy.go 0.00% 1 Missing ⚠️
internal/prompts/app_select.go 96.29% 1 Missing ⚠️
internal/shared/clients.go 50.00% 1 Missing ⚠️
main.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main      #81   +/-   ##
=======================================
  Coverage   63.16%   63.16%           
=======================================
  Files         210      210           
  Lines       22186    22186           
=======================================
+ Hits        14013    14014    +1     
+ Misses       7087     7083    -4     
- Partials     1086     1089    +3     

☔ 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.

Copy link
Member Author

@mwbrooks mwbrooks left a comment

Choose a reason for hiding this comment

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

🎏 A few notes to keep the reviewers company during the long scroll down...

clientsMock.AddDefaultMocks()

clientsMock.AuthInterface.On("ResolveAPIHost", mock.Anything, mock.Anything, mock.Anything).
clientsMock.Auth.On("ResolveAPIHost", mock.Anything, mock.Anything, mock.Anything).
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: Updating our mocks to clientsMock.Auth

// promptTeamSlackAuth retrieves an authenticated team from input
func promptTeamSlackAuth(ctx context.Context, clients *shared.ClientFactory) (*types.SlackAuth, error) {
allAuths, err := clients.AuthInterface().Auths(ctx)
allAuths, err := clients.Auth().Auths(ctx)
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: Updating our client to clients.Auth()

SDKConfig hooks.SDKCLIConfig
API func() api.APIInterface
AppClient func() *app.Client
Auth func() auth.AuthInterface
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: This is the main change

Copy link
Member

Choose a reason for hiding this comment

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

I like that this also removes a space in the struct! 👾 ✨

Comment on lines +147 to +148
// defaultAuthFunc return a new Auth Interface
func (c *ClientFactory) defaultAuthFunc() auth.AuthInterface {
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: Updating the constructor's name

Stdout *bytes.Buffer
HookExecutor hooks.MockHookExecutor
API *api.APIMock
Auth *auth.AuthMock
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: This is the main change to update the mocks.

@mwbrooks mwbrooks marked this pull request as ready for review May 5, 2025 21:25
@mwbrooks mwbrooks requested a review from a team as a code owner May 5, 2025 21:25
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.

@mwbrooks Wow! Between this and #77 the code is reading so much better 📚

The motivation is that the name is shorter and more accurate.

To me this change also makes more sense since I'm often just wanting to call a method from clients and expect it to be through an interface 😉

Thank you yet again for the care of this code 😌 🎉

SDKConfig hooks.SDKCLIConfig
API func() api.APIInterface
AppClient func() *app.Client
Auth func() auth.AuthInterface
Copy link
Member

Choose a reason for hiding this comment

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

I like that this also removes a space in the struct! 👾 ✨

@mwbrooks
Copy link
Member Author

mwbrooks commented May 5, 2025

Thanks for the quick review @zimeg! ❤️ I think we have many more changes like this, but it's nice to see the AuthInterface and APIInterface tidier!

@mwbrooks mwbrooks merged commit f491ca7 into main May 5, 2025
6 checks passed
@mwbrooks mwbrooks deleted the mwbrooks-auth-interface-refactor branch May 5, 2025 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code health M-T: Test improvements and anything that improves code health 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