Improve method return values and scope limiting

[evanofslack]

This PR addresses #56

Summary

Existing methods of client have been modified to return more concrete types in favor of response types.

Other Changes

• Previous response types scoped as public have been made private.
• All response/request types were moved out of the models package and into respective files in the rest package.
• Created types for UserStatus and Room in the models package.
• Added additional tests for im and users.

As some of these are backward incompatible changes, we might want to target a new release to encompass any other additional breaking changes.

[geekgonecrazy]

Why switch from exported to private on all of these?

[evanofslack]

Why switch from exported to private on all of these?

@geekgonecrazy Please correct me if I am wrong, but I cannot see a reason that any user of this SDK or any other package in this library would need to access raw response types. It seems more logical to return a model type and an error compared to a response object with nested model and error.

[debdutdeb]

Hi, sorry to intrude, I was just going through some other pr and this caught my eye.

@evanofslack - if you just return the models, you are misinforming the users about the actual result. For example, the server can return {success: false, error: 'error-i-cant-find'} - this unmarshalled will leave you the model fields with the empty values (e.g. {channels: []}). Even though the variable/field has an empty value, doesn't mean the call was successful and the server just happen to have zero-similar amount of objects registered.

And in the code I don't see the status being checked by the functions themselves either.

You could potentially wrap the error like

if e := resp.Status.Ok(); e != nil { 
    return nil, e
}

But tbh idk if this is something the sdk should do or the user. There is a distinction right now that makes sense and is easier to comprehend imo. Like for the methods themselves if err != nil it is an internal error and someone can just optionally cleanup and panic out without any extra checks. if err == nil they can then check the response status and handle them separately treating as a non-critical error (like error-room-not-found). The separation makes sense to me. Tying them together means the user have to always check for all rocketchat errors before being able to give a 👍🏼 to panic or similar and having to make sure to keep it in sync with any server additions or removals.

[evanofslack]

@debdutdeb Thank you for the feedback.

Maybe I am misunderstanding but it is my interpretation that the client.Get / client.doRequest method will take care of checking the status of the response and return any occurring error through response.OK() after a successful marshall.

func (c *Client) doRequest(...)
        ...
        bodyBytes, err := ioutil.ReadAll(resp.Body)
        var parse bool
    if err == nil {
        if e := json.Unmarshal(bodyBytes, response); e == nil {
            parse = true
        }
    }
    if resp.StatusCode != http.StatusOK {
        if parse {
            return response.OK()
        }
        return errors.New("Request error: " + resp.Status)
    }
       return response.OK()

Then in the specific resource methods, we check if this error occurred and return nil, err accordingly. In this way, the methods always return a model alongside an error.

[debdutdeb]

I see, my misunderstanding then. Thanks

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

---

evan slack seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}