Skip to content
This repository has been archived by the owner on Dec 10, 2024. It is now read-only.

Documentation for ListMergeRequests should be updated #1967

Open
FlorianLoch opened this issue Jul 2, 2024 · 5 comments
Open

Documentation for ListMergeRequests should be updated #1967

FlorianLoch opened this issue Jul 2, 2024 · 5 comments

Comments

@FlorianLoch
Copy link

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.

@FlorianLoch
Copy link
Author

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. 🤔

@svanharmelen
Copy link
Member

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.

@FlorianLoch
Copy link
Author

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...

@svanharmelen
Copy link
Member

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.

@FlorianLoch
Copy link
Author

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. 🤔

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants