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.

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

Inconsistent method return values throughout SDK #56

Open
evanofslack opened this issue Jul 12, 2022 · 1 comment
Open

Inconsistent method return values throughout SDK #56

evanofslack opened this issue Jul 12, 2022 · 1 comment

Comments

@evanofslack
Copy link
Contributor

evanofslack commented Jul 12, 2022

Overview

This issue documents inconsistent and perhaps unideomatic behavior regarding method return values throughout this SDK. Some methods return a resource type while other methods return a higher-level response type. I am proposing that returning the resource type along with an error should be preferable.

I've included some examples of these function signatures below:

Preferable

client.GetMessages has the following signature:

func (c *Client) GetMessages(channel *models.Channel, page *models.Pagination) ([]models.Message, error) 

This is logical, as we expect a message getter function to return a slice of messages and an error.

Unpreferable

Compare this to client.GetPublicChannels with the following signature:

func (c *Client) GetPublicChannels() (*ChannelsResponse, error)

Which returns ChannelsResponse

type ChannelsResponse struct {
	Status
	models.Pagination
	Channels []models.Channel `json:"channels"`
}

Rather than returning a slice of channels, this method returns a response struct with an embedded channels slice. The ChannelsResponse struct needs to exist in order to properly make the HTTP request, but it should not be a public type and it should not be returned from the method. Instead, the channels slice should be returned alongside an error.

There are a few methods currently in the SDK that could be updated to meet this standard (this list is not exshaustive):

Additionally, there are some methods that currently return a response type when it maybe be preferable to return a resource type or even just an error. These are typically cases where resources are being updated or deleted:

Changes like these that modify method signatures are of course breaking changes, so it must be considered whether this is something worth implementing.

@cauefcr
Copy link

cauefcr commented Jul 13, 2022

We should also think about a solution for unifying the realtime and rest clients, if we're talking breaking changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Todo
Development

No branches or pull requests

2 participants