Add ProjectsService #3718
Add ProjectsService #3718
Conversation
|
This PR won't be merged soon because the only one maintainer, @gmlewis, has lost access to the repo. See #3689 (comment) So, let's wait when @google-admin grants access, and then proceed with adding the feature. |
There was a problem hiding this comment.
Thank you, @JoannaaKL!
One tiny tweak, please, and then LGTM.
Could you also please rename the PR from "Add projects" to "Add ProjectsService"?
|
Also, please note that I still don't have write access to this repo, but figure I might as well attempt to catch up on my code reviews (which I usually allow the linter to process first... ah, well). |
| // GitHub API docs: https://docs.github.com/rest/projects/projects#get-project-for-organization | ||
| // | ||
| //meta:operation GET /orgs/{org}/projectsV2/{project_id} | ||
| func (s *ProjectsService) GetByOrg(ctx context.Context, org string, projectID int64) (*ProjectV2, *Response, error) { |
There was a problem hiding this comment.
Maybe GetOrganizationProject is a better name?
There was a problem hiding this comment.
Same as below, GetByID is pretty common in this repo. GetOrganizationProject is redundant - this is the ProjectsService so it's logical it returns a Project. We should not repeat that.
|
|
||
| func (p ProjectV2) String() string { return Stringify(p) } | ||
|
|
||
| // ListProjectsOptions specifies optional parameters to list organization projects. |
There was a problem hiding this comment.
This structure used not only in the list organization project method. So, the comment can be improved.
| // GitHub API docs: https://docs.github.com/en/rest/projects/projects#list-projects-for-user | ||
| // | ||
| //meta:operation GET /users/{username}/projectsV2 | ||
| func (s *ProjectsService) ListByUser(ctx context.Context, username string, opts *ListProjectsOptions) ([]*ProjectV2, *Response, error) { |
There was a problem hiding this comment.
Maybe ListUserProjects is a better name?
There was a problem hiding this comment.
I'd prefer longer name myself but I was following the existing convention - ListByUser, ListByIssue etc.
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| req.Header.Set("Accept", mediaTypeProjectsPreview) |
There was a problem hiding this comment.
Do we need this header?
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| req.Header.Set("Accept", mediaTypeProjectsPreview) |
There was a problem hiding this comment.
Do we need this header?
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| req.Header.Set("Accept", mediaTypeProjectsPreview) |
There was a problem hiding this comment.
Do we need this header?
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| req.Header.Set("Accept", mediaTypeProjectsPreview) |
There was a problem hiding this comment.
Do we need this header?
Co-authored-by: Glenn Lewis <[email protected]>
|
Hey hey @gmlewis 👋 . I renamed the pr, there's one more tiny change I need to do to this pr before it's ready - update the pagination parameters. I hope you get your access back soon! |
Thank you, @JoannaaKL! Me too!!! ❤️ |
First chunk to support new GitHub Projects API