| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
Reviewed the branch locally and ran go test ./pkg/github -run Test_ListIssues (green). Overall the approach looks right: fallback is gated on schema-shape errors only, explicit field_filters are preserved, and the table-driven coverage is good. A few things worth considering: 1. Exact string matching is likely too narrow (main concern)shurcooL/graphql's errors.Error() returns only the first message (graphql.go:117). On a GHES schema missing both dependencies, validation emits several errors, and the first one may not be either of the two whitelisted strings — e.g. InputObject 'IssueFilters' doesn't accept argument 'issueFieldValues', or an Argument 'filterBy' ... has an invalid value variant. In that case no fallback happens and #3068 reproduces on that GHES version. Since the fallback is cheap and safe (any genuine failure resurfaces from the retry), a permissive match seems strictly better: func isUnsupportedListIssuesIssueFieldsError(err error) bool {
msg := err.Error()
return strings.Contains(msg, "IssueFieldValueFilter") || strings.Contains(msg, "issueFieldValues")
}2. Retry error masks the originalIf the fallback query fails for an unrelated reason (rate limit, auth), the user only sees that error and never learns the primary query hit a schema gap. Consider joining both messages. 3. Result extraction via type switch is fragileThe fallback branch already knows the concrete shape, so setting resp/isPrivate there directly would avoid re-detecting it through any with an ordering-sensitive if/else if. 4. The duplicated structs look avoidableAnonymous untagged fields are inlined by the library in both query construction (query.go:115) and unmarshalling (jsonutil/graphql.go:182), so IssueFragment could embed issueFragmentWithoutFieldValues and add only IssueFieldValues, producing an identical query. That removes ~130 lines of duplication plus the field-by-field copy in fragmentToMinimalIssue, which otherwise needs manual updating every time a field is added. 5. Two round trips per call on GHESIncluding every pagination page. A per-client memo of "this schema doesn't support issue fields" would avoid the repeated probe — fine as a follow-up. 6. Scope gapField 'issueFieldValues' doesn't exist on type 'Issue' also affects fetchIssueFieldValuesByNodeID (search_issues), the issue_read enrichment query, and fetchExistingIssueFieldValues — all still hard-fail on GHES. Reasonable to defer, but worth tracking. Nit: getIssueQueryType still returns any while its new sibling returns a typed interface — could be aligned. |
Sorry, something went wrong.
Retry list_issues without custom issue field dependencies only when the host schema lacks them. Preserve explicit field filters and propagate unrelated GraphQL errors. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Handle alternate issue-field validation messages, preserve primary and retry errors, and avoid runtime result type switches. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
| Back | FazBrowse Home | New Git URL |
Summary
Validation
Fixes #3068