-
Notifications
You must be signed in to change notification settings - Fork 24
refactor: rename clients.APIInterface() to clients.API() #77
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #77 +/- ##
==========================================
+ Coverage 63.16% 63.17% +0.01%
==========================================
Files 210 210
Lines 22187 22186 -1
==========================================
+ Hits 14014 14016 +2
Misses 7086 7086
+ Partials 1087 1084 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mwbrooks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments to help clear up some noisy diffs! 🧘🏻
| API func() api.APIInterface | ||
| AppClient func() *app.Client | ||
| AuthInterface func() auth.AuthInterface | ||
| CLIVersion string | ||
| Config *config.Config | ||
| EventTracker tracking.TrackingManager | ||
| HookExecutor hooks.HookExecutor | ||
| IO iostreams.IOStreamer | ||
| Runtime runtime.Runtime | ||
| SDKConfig hooks.SDKCLIConfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are 3 changes here:
- Renamed
APIInterface → API - Alphabetically ordered these fields
- Removed newline breaks and unnecessary comments so indentation aligns
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meant to send this with the review prior, but thank you so very much! This is a great PR to make such a change 😌 🎉
zimeg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mwbrooks Less code is better so often! 🏆
While we're updating this would it be best to make a similar change to tests?
slack-cli/internal/shared/clients_mock.go
Line 35 in aabed5e
| APIInterface *api.APIMock |
| type ClientsMock struct { | ||
| mock.Mock | ||
| APIInterface *api.APIMock | ||
| API *api.APIMock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: Update ClientsMock.APIInterface to ClientsMock.API in commit 54da648
@zimeg Great catch! I'm a little disappointed in myself for not catching this - I looked through the test and mocks, but overlooked this change. Commit 54da648 updates the |
zimeg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mwbrooks You are so fast! 👾 ✨
All of these changes look great and I'm super excited to be using a more refined comments and clients in calling code 👁️🗨️
|
Back at you @zimeg! Thanks for taking the time to review these tedious changes! Ever-so-slowly, things look and feel better. I'll merge this PR and follow-up with one that updates |
Summary
This pull request renames the function
clients.APIInterface()toclients.API().The motivation is that the name is shorter and more accurate. The
clients.API()returns an instance ofapi.Clientthat implements theapi.APIInterfaceinterface. It doesn't return an interface. Alternatively, we could name itclients.APIClient()but I think we're trying to move away from this naming pattern.There was no change functionality was changed.
Requirements