Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add optional team parameter to GetTeamInfo function calls #981

Closed
wants to merge 1 commit into from
Closed

Add optional team parameter to GetTeamInfo function calls #981

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 10 additions & 3 deletions team.go
View file
Open in desktop
Expand Up @@ -96,15 +96,22 @@ func (api *Client) accessLogsRequest(ctx context.Context, path string, values ur
}

// GetTeamInfo gets the Team Information of the user
func (api *Client) GetTeamInfo() (*TeamInfo, error) {
return api.GetTeamInfoContext(context.Background())
// if the team is empty (""), will return information about the current team.
// https://api.slack.com/methods/team.info
func (api *Client) GetTeamInfo(team string) (*TeamInfo, error) {
Copy link

Choose a reason for hiding this comment

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

Backward compatibility issue.

Copy link
Member

@kanata2 kanata2 Nov 6, 2021 •
edited

Choose a reason for hiding this comment

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

I'm having trouble with only one optional parameter...
I've come up with a few ideas, which one do you think is better?

other idea 1

How about adding GetAnotherTeamInfo(team string) method .

// Get your own team information.
info, err := api.GetTeamInfo()

// Get other team information.
info, err := api.GetOtherTeamInfo(teamID)

other idea 2

Use functional option pattern.

// Get your own team information.
info, err := api.GetTeamInfo()

// Get other team information.
info, err := api.GetTeamInfo(slack.GetTeamInfoOptionTeamID(teamID))

Copy link
Author

Choose a reason for hiding this comment

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

I feel like even though this is not backwards compatible, it follows how the slack API actually operates and makes it less confusing.

From Slack API Docs:

Team to get info on, if omitted, will return information about the current team. Will only return team that the authenticated token is allowed to see through external shared channels

I think that other option 2 is closer to the APIs definition, but not nearly as close as my proposal.

Thoughts?

Copy link
Member

@kanata2 kanata2 Dec 9, 2021 •
edited

Choose a reason for hiding this comment

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

I think that other option 2 is closer to the APIs definition, but not nearly as close as my proposal.

The parameter team is an optional in Slack while it is required in our GetTeamInfo API with your proposal, so I think it is not so good design.
The difference in idea 2 is that it keeps team parameter optional.

return api.GetTeamInfoContext(context.Background(), team)
}

// GetTeamInfoContext gets the Team Information of the user with a custom context
func (api *Client) GetTeamInfoContext(ctx context.Context) (*TeamInfo, error) {
// if the team is empty (""), will return information about the current team.
// https://api.slack.com/methods/team.info
func (api *Client) GetTeamInfoContext(ctx context.Context, team string) (*TeamInfo, error) {
values := url.Values{
"token": {api.token},
}
if team != "" {
values.Add("team", team)
}

response, err := api.teamRequest(ctx, "team.info", values)
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion team_test.go
View file
Open in desktop
Expand Up @@ -33,7 +33,7 @@ func TestGetTeamInfo(t *testing.T) {
once.Do(startServer)
api := New("testing-token", OptionAPIURL("http://"+serverAddr+"/"))

teamInfo, err := api.GetTeamInfo()
teamInfo, err := api.GetTeamInfo("F0UWHUX")
if err != nil {
t.Errorf("Unexpected error: %s", err)
return
Expand Down