-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat!: Add support for project items CRUD and project fields read operations #3793
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
base: master
Are you sure you want to change the base?
feat!: Add support for project items CRUD and project fields read operations #3793
Conversation
Co-authored-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>
…into add-project-items
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
add-project-items branch and adds support for field retrievaladd-project-items branch with master and implements field retrieval support
add-project-items branch with master and implements field retrieval supportadd-project-items branch with master and adds field retrieval support
Thanks @gmlewis, updated the PR title and PR description, hoping that's a little more clear, let me know if you have any questions. |
So the state of the external-to-this-repo branch called "JoannaaKL:stephenotalora/add-project-items" is irrelevant to this PR and all that information in the description and in the PR title should be removed. We try to make the PR title helpful to understand what this PR is all about. How about this: "feat!: Add methods to edit ProjectsV2 fields" The "feat!:" part is needed because this PR breaks the API from previous versions. |
add-project-items branch with master and adds field retrieval supportThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your PR title now says "GET" but this PR also includes "POST", "PATCH", and "DELETE" methods, so please make the PR title reflect this.
Before I proceed with this PR review, it looks like this PR has some regressions that were already addressed in #3718.
github/projects.go
Outdated
| UpdatedAt *Timestamp `json:"updated_at,omitempty"` | ||
| DeletedAt *Timestamp `json:"deleted_at,omitempty"` | ||
| Number *int `json:"number,omitempty"` | ||
| Number *int64 `json:"number,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a discussion about this field being of type int because we felt that a project number would not overflow a 32-bit integer.
Why are you changing this here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you changing this here?
Thanks @gmlewis, I appreciate you resurfacing the previous discussion; I had missed that context earlier. A couple of points that led to this change:
- I took over the base PR, at first glance it appeared there was an intentional effort to transition the
projectNumberfield to anint64. - To validate the assumption:
- I checked our internal database schema and found the field defined as an
UNSIGNED BIGINT - Additionally, the OpenAPI specification lists this field as an
int64 - Both of which reinforced the idea that a wider numeric type might be appropriate in Go.
- I checked our internal database schema and found the field defined as an
On second thought, though, an UNSIGNED BIGINT doesn’t directly map to a Go int64, since their ranges differ, while a uint64 would technically align more closely, as you described here that would likely add unnecessary complexity given the low risk of overflow for project numbers.
With that in mind, retaining the int type seems appropriate given the prior context. I can update the PR accordingly 👍🏼
updated: fixed in 25561d3
github/projects.go
Outdated
| Color string `json:"color,omitempty"` | ||
| // An optional description for this option. | ||
| Description string `json:"description,omitempty"` | ||
| ID *int64 `json:"id,omitempty"` // The unique identifier for this option. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this being changed from a string?
Actually, it should probably be a *string here if it truly is an optional field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
github/projects.go
Outdated
| Name string `json:"name,omitempty"` // The display name of the option. | ||
| Color string `json:"color,omitempty"` // The color associated with this option (e.g., "blue", "red"). | ||
| Description string `json:"description,omitempty"` // An optional description for this option. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional fields should be pointers to strings in general. I'm not sure why these were left as values in #3718.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 27dd1c4
github/projects.go
Outdated
| // | ||
| //meta:operation GET /orgs/{org}/projectsV2/{project_number} | ||
| func (s *ProjectsService) GetProjectForOrg(ctx context.Context, org string, projectNumber int) (*ProjectV2, *Response, error) { | ||
| func (s *ProjectsService) GetProjectForOrg(ctx context.Context, org string, projectNumber int64) (*ProjectV2, *Response, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and below, projectNumber should remain as int unless you have a good reason to change them in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thank you 🙇🏼 I went through a rather large conflict earlier, and the title reflected the later additions, that’s my oversight. I’ll update accordingly. update: I've taken the PR back to a draft while I work on edits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @stephenotalora.
github/projects.go
Outdated
| // GitHub API docs: https://docs.github.com/rest/projects/fields#list-project-fields-for-user | ||
| // | ||
| //meta:operation GET /users/{username}/projectsV2/{project_number}/fields | ||
| func (s *ProjectsService) ListProjectFieldsForUser(ctx context.Context, user string, projectNumber int, opts *ListProjectsOptions) ([]*ProjectV2Field, *Response, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I seem to recall a conversation we had earlier about changing "*ForUser" to (for example) "ListUserProjectFields" but looking through this go-github repo, I see we have both styles and I can't seem to find the conversation, so I'll leave this now and maybe our other reviewers will have an opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to make this change if there’s a strong preference either way. It would be a breaking change on our end, but easy enough to address. I did a quick search across the repo, and it looks like *ForUser and *ForOrg are used fairly extensively, in about 4 and 12 files, respectively.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| func (s *ProjectsService) ListProjectFieldsForUser(ctx context.Context, user string, projectNumber int, opts *ListProjectsOptions) ([]*ProjectV2Field, *Response, error) { | |
| func (s *ProjectsService) ListUserProjectFields(ctx context.Context, user string, projectNumber int, opts *ListProjectsOptions) ([]*ProjectV2Field, *Response, error) { |
See #3761
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @stephenotalora!
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.
cc: @stevehipwell - @alexandear - @zyfy29
github/projects.go
Outdated
| // GitHub API docs: https://docs.github.com/rest/projects/fields#get-project-field-for-organization | ||
| // | ||
| //meta:operation GET /orgs/{org}/projectsV2/{project_number}/fields/{field_id} | ||
| func (s *ProjectsService) GetProjectFieldForOrg(ctx context.Context, org string, projectNumber int, fieldID int64) (*ProjectV2Field, *Response, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| func (s *ProjectsService) GetProjectFieldForOrg(ctx context.Context, org string, projectNumber int, fieldID int64) (*ProjectV2Field, *Response, error) { | |
| func (s *ProjectsService) GetOrganizationProjectField(ctx context.Context, org string, projectNumber int, fieldID int64) (*ProjectV2Field, *Response, error) { |
See #3761
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for feedback @alexandear, I'll start working on these changes shortly 👍🏼
update: refactored as part of c2f8bfa
github/projects.go
Outdated
| // GitHub API docs: https://docs.github.com/rest/projects/fields#get-project-field-for-user | ||
| // | ||
| //meta:operation GET /users/{username}/projectsV2/{project_number}/fields/{field_id} | ||
| func (s *ProjectsService) GetProjectFieldForUser(ctx context.Context, user string, projectNumber int, fieldID int64) (*ProjectV2Field, *Response, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| func (s *ProjectsService) GetProjectFieldForUser(ctx context.Context, user string, projectNumber int, fieldID int64) (*ProjectV2Field, *Response, error) { | |
| func (s *ProjectsService) GetUserProjectField(ctx context.Context, user string, projectNumber int, fieldID int64) (*ProjectV2Field, *Response, error) { |
See #3761
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refactored as part of c2f8bfa
github/projects.go
Outdated
| // GitHub API docs: https://docs.github.com/rest/projects/items#add-item-to-organization-owned-project | ||
| // | ||
| //meta:operation POST /orgs/{org}/projectsV2/{project_number}/items | ||
| func (s *ProjectsService) AddProjectItemForOrg(ctx context.Context, org string, projectNumber int, opts *AddProjectItemOptions) (*ProjectV2Item, *Response, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| func (s *ProjectsService) AddProjectItemForOrg(ctx context.Context, org string, projectNumber int, opts *AddProjectItemOptions) (*ProjectV2Item, *Response, error) { | |
| func (s *ProjectsService) AddOrganizationProjectItem(ctx context.Context, org string, projectNumber int, opts *AddProjectItemOptions) (*ProjectV2Item, *Response, error) { |
See #3761
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refactored as part of 7f8c2f9
…ed naming convention introduced in google#3761 for consistency across the client API.
…ing convention defined in google#3761
…d naming convention defined in google#3761
|
Thank you @gmlewis and @alexandear for taking the time to review this PR; I really appreciate your feedback and attention to detail 🙏🏼 I’ve incorporated the recent suggestions, particularly aligning the function naming conventions for I also wanted to ask if there's a general expectation regarding the merge and release timeline for this PR. I understand schedules can vary, but having an approximate idea would help with planning once the new APIs become available. No rush; I just wanted to check in and thank you both again for your time and for maintaining such a valuable library! |
I can cut a release once this gets merged, no problem.
Thank you for your patience during the open source review process, and for your contributions! |
BREAKING CHANGES:
ProjectV2Field.Optionschanged from[]anyto[]*ProjectV2FieldOption.ProjectV2structs are now passed as pointersProjectsServicefunctions with Consistent naming for enterprise & organization scoped functions #3761Closes: #3733.
Based on: #3733
Covers all endpoints for project items : https://docs.github.com/en/rest/projects/items?apiVersion=2022-11-28
Adds support to GET fields on project items, specifically the following REST APIs: