Address projects bugs #7007
Address projects bugs #7007
Conversation
| @@ -42,45 +42,6 @@ func OrganizationProjects(client *Client, repo ghrepo.Interface) ([]RepoProject, | |||
| return projects, nil | |||
| } | |||
|
|
|||
| // OrganizationProjectsV2 fetches all open projectsV2 for an organization. | |||
| func OrganizationProjectsV2(client *Client, repo ghrepo.Interface) ([]RepoProjectV2, error) { | |||
There was a problem hiding this comment.
This just got moved not actually deleted. Consolidated it in queries_projects_v2.go.
api/queries_repo.go
Outdated
| count := 0 | ||
| var result RepoMetadataResult | ||
|
|
||
| g, _ := errgroup.WithContext(context.Background()) |
There was a problem hiding this comment.
errgroup felt like a nice improvement here.
There was a problem hiding this comment.
Agreed. Nit: since we don't have a parent context here, and we won't use the child context for anything either, you can just initialize a Group without context.
| g, _ := errgroup.WithContext(context.Background()) | |
| var g errgroup.Group |
| @@ -1096,87 +1067,6 @@ func RepoProjects(client *Client, repo ghrepo.Interface) ([]RepoProject, error) | |||
| return projects, nil | |||
| } | |||
|
|
|||
| // RepoProjectsV2 fetches all open projectsV2 for a repository. | |||
| func RepoProjectsV2(client *Client, repo ghrepo.Interface) ([]RepoProjectV2, error) { | |||
There was a problem hiding this comment.
Moved to queries_projects_v2.go
| @@ -1050,14 +1029,6 @@ type RepoProject struct { | |||
| ResourcePath string `json:"resourcePath"` | |||
| } | |||
|
|
|||
| type RepoProjectV2 struct { | |||
There was a problem hiding this comment.
Moved to queries_projects_v2.go
| @@ -96,6 +104,14 @@ func (pr *PullRequest) ExportData(fields []string) map[string]interface{} { | |||
| data[f] = pr.Labels.Nodes | |||
| case "projectCards": | |||
| data[f] = pr.ProjectCards.Nodes | |||
| case "projectItems": | |||
There was a problem hiding this comment.
We usually have these field names map closely to the underlying GraphQL schema, but that's not a strict contract. In this case, I wonder if the projectCards vs. projectItems distinction does any service to the user. Offering both without any documentation clarifying that one is Projects Classic and another is Projects Next could be confusing.
Could we just merge ProjectItems and expose it under projectCards?
There was a problem hiding this comment.
Agreed that the distinction is confusing. We definitely could merge the two. I think there are two downsides.
The first is that new projects don't use "cards", so it might be confusing for users who are familiar with only projects V2.
Second, is that the data we want to return for each is slightly different. They have the title in common but that is about it. What if in the future we want to expand on what data gets returned for projectItems? I don't envision projectCards getting expanded as projects classic usage dwindles.
I think if we didn't have to worry about backwards compatibility I would suggest making a new projects name that would encompass both, while removing projectCards. Since we do, I would perhaps leave it as projectCards and projectItems while adding some help documentation describing the difference.
There was a problem hiding this comment.
Good point about "cards" not being a term that new-style Projects use. Now that you reasoned about this, I see value in projectCards and projectItems being distinct. I wonder if we could somehow provide some documentation for our --json fields noting that projectCards is basically a legacy API and that most users will want projectItems for their generic Projects-related needs 🤔
| "pageInfo": { "hasNextPage": false } | ||
| } } } } | ||
| `)) | ||
| mockRetrieveProjects(t, reg) |
api/queries_repo.go
Outdated
| count := 0 | ||
| var result RepoMetadataResult | ||
|
|
||
| g, _ := errgroup.WithContext(context.Background()) |
There was a problem hiding this comment.
Agreed. Nit: since we don't have a parent context here, and we won't use the child context for anything either, you can just initialize a Group without context.
| g, _ := errgroup.WithContext(context.Background()) | |
| var g errgroup.Group |
Fixes #6989 - Allow retrieving
projectItemsfrom--jsonflag.Fixes #6940 - Retrieve current user projectsV2 along with projectsV2 we were already retrieving.
Note that for now I did not address the behavior of projects with duplicate names. I think we should hold off on that for now as any behavior that I experimented with felt confusing. When there are duplicate named projects there is no way to know which project a user is referring to. I think our advice for users in this situation should be to rename the project to a unique name and/or manipulate the project on the web. This edge case seems highly unlikely as we have never (as far as I know) run into it with projects classic. If we have reports of this actually occurring we can revisit my decision.