RocketChat / Rocket.Chat.Go.SDK

Go SDK for REST API and Realtime api
MIT License
59 stars 58 forks source link

Inconsistent method return values throughout SDK #56

Open evanofslack opened 2 years ago

evanofslack commented 2 years ago

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 commented 2 years ago

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