Documentation for `ListMergeRequests` should be updated #1967

Open FlorianLoch opened 2 weeks ago

ListMergeRequests returns []*MergeRequest - which is tricky, as - according to GitLab's example and real-world experience - the response of the API is not fully populated.

That results in not fully populated instances of MergeRequest.

head_pipeline is an example for that.

I would therefore suggest to either create another type representing just the subset of fields that actually get returned or to at least state it in the docs. Because like this it's hard to tell whether a field is indeed empty/false/0 or whether it's just the zero value.

I am happy to provide an MR for this.

Having an additional type might be the nicer solution as someone not reading the documentation will not run into issues due to the zero-valued fields - but it will break the API. 🤔

Hi @FlorianLoch, thanks for your suggestion but I'm not sure if I'm willing to go there... Can you see (in the GitLab source code) if there is actually a dedicated type used for list call (or at least this list call)? My worry is that it might not be clearly written down which fields are returned when. And if that is not clear, then there is no sensible way to support it.

I tried investigating this in GitLab's source code - but I couldn't find anything... Ruby being very implicit and me not having use it for ages didn't really help 🙈

Trying to understand what is going on in https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/models/merge_request.rb#L378 I found "some evidence" that only a set of associated entities are populated - but no proper list/type...

So unless someone more skilled in reading the GitLab codebase finds something actually useful I would suggest to go with option two: mention in the libraries documentation, that the type will not come fully populated and that one should check the upstream documentation.

That isn't really satisfying, but at least these fields being empty does not come as such a big surprise anymore...

I'm open to something like this:

// ListCommitsOptions represents the available ListCommits() options.
//
// Note: Some of the fields might not be populated by the GitLab API.
//
// GitLab API docs: https://docs.gitlab.com/ee/api/commits.html#list-repository-commits

If applied consistently to all functions that this applies to. Not sure how valuable it would be, but it doesn't hurt either.

I would consider this a good addition. Of course, it isn't perfect as it doesn't help the developer who is coding on autocomplete and simple expects all fields to be present - but it will help the ones reading a little more of the code they are using 😉

The remaining question would then be how to find other functions that are "affected" by this. 🤔

xanzy / go-gitlab

Documentation for `ListMergeRequests` should be updated #1967