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

Return basic info about pipeline in GET /projects/:id/pipelines #163

Merged
merged 3 commits into from
Apr 4, 2017
Merged

Return basic info about pipeline in GET /projects/:id/pipelines #163

merged 3 commits into from
Apr 4, 2017

Conversation

ivaravko
Copy link
Contributor

@ivaravko ivaravko commented Apr 1, 2017

Copy link
Member

@svanharmelen svanharmelen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for helping to get stuff ready for the V4 API! I do have some comments/suggestions on this PR. Please let me know your thoughts...

git.SetBaseURL("https://gitlab.com/api/v4")

pipelines, _, err := git.Pipelines.ListProjectPipelines(2743054)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitting please remove this empty line here

Ref string `json:"ref"`
Sha string `json:"sha"`
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what to make of this. I see what you are trying to do and I agree it's maybe somewhat cleaner to have a smaller struct matching the field returned from a list call. But I don't think I like the suggested solution.

The name RootPipeline seems to imply something that doesn't exist (there is no such thing as a root of a pipeline). And also please do not put the new type in between the Pipeline type and it's String method. I like to keep those close together. Additionally please also add the String method for the new struct.

As for a possible solution I would like to suggest making a PipelineList type:

type PipelineList []struct {
    ID     int    `json:"id"`
    Status string `json:"status"`
    Ref    string `json:"ref"`
    Sha    string `json:"sha"`
}

I think that would (both in term of it's name and it's structure) fit the context a bit better. Thoughts?

@svanharmelen
Copy link
Member

@ivaravko that looks much better (to me at least 😉) Thanks!

@svanharmelen svanharmelen merged commit c806186 into xanzy:f-api-v4 Apr 4, 2017
@johanbrandhorst johanbrandhorst mentioned this pull request Apr 4, 2017
39 tasks
@ivaravko ivaravko deleted the f-api-v4-pipelines branch April 4, 2017 18:44
svanharmelen pushed a commit that referenced this pull request Aug 25, 2017
* Return basic info about pipeline in GET /projects/:id/pipelines

* Adds pipeline example

* Rename RootPipeline to PipelineList with type []struct
svanharmelen pushed a commit that referenced this pull request Aug 25, 2017
* Return basic info about pipeline in GET /projects/:id/pipelines

* Adds pipeline example

* Rename RootPipeline to PipelineList with type []struct
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants