Issue commands should parse args early #10811
Conversation
| "github.com/cli/cli/v2/pkg/set" | ||
| "golang.org/x/sync/errgroup" | ||
| ) | ||
|
|
||
| func ParseIssuesFromArgs(args []string) ([]int, o.Option[ghrepo.Interface], error) { |
There was a problem hiding this comment.
This is really the same logic as IssueFromArgWithFields without actually fetching the PR, and trying to indicate a little bit more with the types what valid states exist.
| @@ -142,6 +221,37 @@ type PartialLoadError struct { | |||
| error | |||
| } | |||
|
|
|||
| func FindIssuesOrPRs(httpClient *http.Client, repo ghrepo.Interface, issueNumbers []int, fields []string) ([]*api.Issue, error) { | |||
There was a problem hiding this comment.
This is the content of IssuesFromArgWithFields without the parse step, so it operates directly on issue numbers and an already determined repo.
| @@ -72,7 +72,19 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(*EditOptions) error) *cobra.Comman | |||
| // support `-R, --repo` override | |||
| opts.BaseRepo = f.BaseRepo | |||
|
|
|||
| opts.SelectorArgs = args | |||
| issueNumbers, baseRepo, err := shared.ParseIssuesFromArgs(args) | |||
There was a problem hiding this comment.
This is really where the magic happens, we move arg parsing into the pre-run, which means we can inject the correct issue numbers and base repo.
Since we now have the base repo, we are able to construct a featuredetection.Detector early to address things like https://github.com/cli/cli/pull/10785/files#r2046543890
There was a problem hiding this comment.
thought: looking at this, I was wondering how gh issue edit handled multiple URLs from different repositories, which apparently raises an error they need to be in the same repo:
$ gh issue edit https://github.com/tinyfists/gh-nonsense-internal/issues/37 https://github.com/tinyfists/gh-nonsense/issues/7 --title "Same same but different"
multiple issues must be in same repo: found "tinyfists/gh-nonsense", expected "tinyfists/gh-nonsense-internal"that really isn't apparent from the --help usage docs.
| return issueNumber, o.None[ghrepo.Interface](), nil | ||
| } | ||
|
|
||
| type issueLocator struct { |
There was a problem hiding this comment.
Bit meh. Might remove in favour of two returns, it's just that I don't want to indicate a valid state that one might be present but the other isn't.
There was a problem hiding this comment.
thought: if this was exported, then I might feel differently, but as-is I think I'm in favor of 2 returns just because of its limited use in parseIssueFromArg. 🤷
| } | ||
|
|
||
| // tryParseIssueFromURL tries to parse an issue number and repo from a URL. | ||
| func tryParseIssueFromURL(maybeURL string) o.Option[issueLocator] { |
There was a problem hiding this comment.
tryParseIssueLocator maybe. Might just remove the type though.
| return o.None[issueLocator]() | ||
| } | ||
|
|
||
| if u.Scheme != "https" && u.Scheme != "http" { |
There was a problem hiding this comment.
Bare numbers parse as valid URL, so I guess this is checking against that. Probably needs a comment.
| baseRepo, err := opts.BaseRepo() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
question: how do these changes reduce the need for passing featuredetection down to findIssueOrPR for GHES v1 projects fetching?
making sure I'm connecting the dots, all of the RunE changes before we get here potentially allow us to resolve the base repo, which is needed for featuredetection initialization as mentioned here in cli/cli#10785.
however, I'm having difficulty imagining how this reduces the work required to pass it down the stack to get to shared.findIssueOrPR() 🤔
There was a problem hiding this comment.
That's a great question, which is lead by what's on the branch. I want to push the decision about inclusion of projects and projectCards up the stack so that instead of "adding it and then later removing it deep down in the stack", we just don't add it.
The advantage to this is that all uses of v1 projects become very clear when we come to remove them. It also avoids having some dependencies passed around that are only used in rare cases, and the mental load of carrying that round.
I think it'll become clearer if I take this work just a little further.
There was a problem hiding this comment.
The advantage to this is that all uses of v1 projects become very clear when we come to remove them. It also avoids having some dependencies passed around that are only used in rare cases, and the mental load of carrying that round.
Driving home this point, while looking at the work on this branch from November, I had to work my way back up each call stack to be sure that I understood all the places feature detection was being wired through, and whether it was having some kind of effect.
There was a problem hiding this comment.
I think it'll become clearer if I take this work just a little further.
Yeah, I would like to see the changes as I'm having a hard time imagining how we avoid passing anything new down the stack.
There was a problem hiding this comment.
There was a problem hiding this comment.
Oh yeah, muuuuuuuuch better! ✨ 💯
There was a problem hiding this comment.
thought: I wonder if there should be an initializer for fd instead of copy pasta the same logic everywhere.
| // add the issue number to the list | ||
| issueNumbers[i] = issueNumber |
There was a problem hiding this comment.
General question; what about duplicate issue numbers? We can drop them here, or let the invoked command handle it. I think the latter is the current behaviour.
There was a problem hiding this comment.
With regards this PR I have no opinion. Currently I think you'll just end up editing the same issue twice, interactively, for issue edit.
1558a93 to
112701e
Compare
112701e to
cfa90a2
Compare
| @@ -37,15 +38,41 @@ func NewCmdComment(f *cmdutil.Factory, runF func(*prShared.CommentableOptions) e | |||
| Args: cobra.ExactArgs(1), | |||
| PreRunE: func(cmd *cobra.Command, args []string) error { | |||
| opts.RetrieveCommentable = func() (prShared.Commentable, ghrepo.Interface, error) { | |||
| // TODO wm: more testing | |||
There was a problem hiding this comment.
This block is totally untested. No BA4A t sure how much I want to tackle this for the extra confidence. TBD.
| tests := []struct { | ||
| name string | ||
| input string | ||
| expectedissueNumber int |
| // TODO: remember why we do this | ||
| cmd.Flags().BoolP("help", "x", false, "") |
There was a problem hiding this comment.
I'd rather see an explanation like: "this is to avoid collision with -h usage for anything other than --help; like -h for hostname". Also, what if the command under test had an x flag/option? Can't we use a safer shorthand instead of x, like _ or ?? I haven't tested these chars with Cobra but as I checked its code, seems like there's no restriction on the chars.
There was a problem hiding this comment.
Tbh I just didn't remember why we were doing this, but I think you've remembered for me. I think we should probably extract something shared like avoidHelpCollision(cmd).
|
|
||
| func issueNumberFromOpts(t *testing.T, v any) int { | ||
| rv := reflect.ValueOf(v) | ||
| field := rv.FieldByName("IssueNumber") |
There was a problem hiding this comment.
I know this probably is the best way of doing this, so I'm just thinking out loud and these are just half-baked and over-engineered solutions. Feel free to ignore with no reply.
I'm thinking of two possible solutions to avoid binding to field names
1. Field tags
We can loop through the fields and pick the one with the right tag. We're still binding to a string literal, but the field name is free of any restriction.
type FooOption struct {
BaseRepo func() (ghrepo.Interface, error) `ghopt:"baseRepo"`
IssueNumber int `ghopt:"issueNumber"`
}2. Custom funcs
We can to add two func parameters to TestArgParsing to be responsible for extracting issue number and base repo:
extractIssueNumber:func(opt T) intextractBaseRepo:func(opt T) ghrepo.Interface
If the passed argument is nil, we can use the current PR's implementation as a fallback. This way TestArgParsing is flexible enough for various situations.
There was a problem hiding this comment.
I don't like 1 at all because it makes changes to production code to satisfy the test.
Re: 2, I don't really want commands to deviate from the pattern without good reason, so having configurability isn't an advantage to me.
There was a problem hiding this comment.
Note because Go generics are not very powerful, we can't constrain T to struct { IssueNumber int }, so there's no way to get type safety through this. Reflection is the only way.
There was a problem hiding this comment.
True. Unless we introduce a getter function that we can abstract behind an interface (like IssueNumberRequired, or something).
There was a problem hiding this comment.
Totally, and I don't want to do that either for the same reason as 1.
| func TestNewCmdDelete(t *testing.T) { | ||
| argparsetest.TestArgParsing(t, NewCmdDelete) | ||
| } |
| { | ||
| name: "issue number argument", | ||
| args: "451 --repo owner/repo", | ||
| want: LockOptions{ | ||
| IssueNumber: 451, | ||
| }, | ||
| }, | ||
| { | ||
| name: "argument is hash prefixed number", | ||
| // Escaping is required here to avoid what I think is shellex treating it as a comment. | ||
| args: "\\#451 --repo owner/repo", | ||
| want: LockOptions{ | ||
| IssueNumber: 451, | ||
| }, | ||
| }, | ||
| { | ||
| name: "argument is a URL", | ||
| args: "https://github.com/cli/cli/issues/451", | ||
| want: LockOptions{ | ||
| IssueNumber: 451, | ||
| }, | ||
| }, | ||
| { | ||
| name: "argument cannot be parsed to an issue", | ||
| args: "unparseable", | ||
| wantErr: "invalid issue format: \"unparseable\"", | ||
| }, |
There was a problem hiding this comment.
Why not re-using TestArgParsing?
There was a problem hiding this comment.
Probably can, I think I didn't because unlock requires parentCmd to be set: https://github.com/cli/cli/pull/10811/files/55d3b1eaa4e27acad346707ab91c9eb0e4a32a71#diff-c341e36bd18bb73313231c0bda8d126954f03bd766117351db33a1dba406a185R467.
Happy to revisit lock and keep unlock separate.
| return o.None[issueLocator]() | ||
| } | ||
|
|
||
| if u.Scheme != "https" && u.Scheme != "http" { |
| wantIssue: 13, | ||
| wantProjects: "", | ||
| wantRepo: "https://example.org/OWNER/REPO", | ||
| behavior: "when given a URL that isn't an issue or PR url, errors", |
There was a problem hiding this comment.
we need one more test case to verify inputs like /cli/cli/issues/999 cause error (url.Parse parses that successfully).
|
In light of the fact that this PR sits at the base of a large stack that I don't want to rebase, after talking with @babakks, I am going to merge this PR and address the comment in a follow up. |
This MR contains the following updates: | Package | Update | Change | |---|---|---| | [cli/cli](https://github.com/cli/cli) | minor | `v2.69.0` -> `v2.72.0` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>cli/cli (cli/cli)</summary> ### [`v2.72.0`](https://github.com/cli/cli/releases/tag/v2.72.0): GitHub CLI 2.72.0 [Compare Source](cli/cli@v2.71.2...v2.72.0) ####Accessibility public preview This release marks the public preview of several accessibility improvements to the GitHub CLI that have been under development over the past year in partnership with our friends at [Charm](https://github.com/charmbracelet) including: - customizable and contrasting colors - non-interactive user input prompting - text-based spinners These new experiences are captured in a new `gh a11y` help topic command, which goes into greater detail into the motivation behind each of them as well as opt-in configuration settings / environment variables. We would like you to share your feedback and join us on this journey through one of [GitHub Accessibility feedback channels](https://accessibility.github.com/feedback)! 🙌 #### What's Changed ##### ✨ Features - Introduce `gh accessibility` help topic highlighting GitHub CLI accessibility experiences by [@​andyfeller](https://github.com/andyfeller) in cli/cli#10890 - \[gh pr view] Support `closingIssuesReferences` JSON field by [@​iamazeem](https://github.com/iamazeem) in cli/cli#10544 ##### 🐛 Fixes - Fix expected error output of `TestRepo/repo-set-default` by [@​aconsuegra](https://github.com/aconsuegra) in cli/cli#10884 - Ensure accessible password and auth token prompters disable echo mode by [@​andyfeller](https://github.com/andyfeller) in cli/cli#10885 - Fix: Accessible multiselect prompt respects default selections by [@​BagToad](https://github.com/BagToad) in cli/cli#10901 #### New Contributors - [@​aconsuegra](https://github.com/aconsuegra) made their first contribution in cli/cli#10884 **Full Changelog**: cli/cli@v2.71.2...v2.72.0 ### [`v2.71.2`](https://github.com/cli/cli/releases/tag/v2.71.2): GitHub CLI 2.71.2 [Compare Source](cli/cli@v2.71.1...v2.71.2) #### What's Changed - Fix pr create when push.default tracking and no merge ref by [@​williammartin](https://github.com/williammartin) in cli/cli#10863 **Full Changelog**: cli/cli@v2.71.1...v2.71.2 ### [`v2.71.1`](https://github.com/cli/cli/releases/tag/v2.71.1): GitHub CLI 2.71.1 [Compare Source](cli/cli@v2.71.0...v2.71.1) #### What's Changed - Fix pr create when branch name contains slashes by [@​williammartin](https://github.com/williammartin) in cli/cli#10859 **Full Changelog**: cli/cli@v2.71.0...v2.71.1 ### [`v2.71.0`](https://github.com/cli/cli/releases/tag/v2.71.0): GitHub CLI 2.71.0 [Compare Source](cli/cli@v2.70.0...v2.71.0) #### What's Changed ##### ✨ Features - `gh pr create`: Support Git's `@{push}` revision syntax for determining head ref by [@​BagToad](https://github.com/BagToad) in cli/cli#10513 - Introduce option to opt-out of spinners by [@​BagToad](https://github.com/BagToad) in cli/cli#10773 - Update configuration support for accessible colors by [@​andyfeller](https://github.com/andyfeller) in cli/cli#10820 - `gh config`: add config settings for accessible prompter and disabling spinner by [@​BagToad](https://github.com/BagToad) in cli/cli#10846 ##### 🐛 Fixes - Fix multi pages search for gh search by [@​leudz](https://github.com/leudz) in cli/cli#10767 - Fix: `project` commands use shared progress indicator by [@​BagToad](https://github.com/BagToad) in cli/cli#10817 - Issue commands should parse args early by [@​williammartin](https://github.com/williammartin) in cli/cli#10811 - Feature detect v1 projects on `issue view` by [@​williammartin](https://github.com/williammartin) in cli/cli#10813 - Feature detect v1 projects on non web-mode `issue create` by [@​williammartin](https://github.com/williammartin) in cli/cli#10815 - Feature detect v1 projects on web mode issue create by [@​williammartin](https://github.com/williammartin) in cli/cli#10818 - Feature detect v1 projects on issue edit by [@​williammartin](https://github.com/williammartin) in cli/cli#10819 ##### 📚 Docs & Chores - Refactor Sigstore verifier logic by [@​malancas](https://github.com/malancas) in cli/cli#10750 #####
Dependencies - chore(deps): bump github.com/sigstore/sigstore-go from 0.7.1 to 0.7.2 by [@​dependabot](https://github.com/dependabot) in cli/cli#10787 - Bump google.golang.org/grpc from 1.71.0 to 1.71.1 by [@​dependabot](https://github.com/dependabot) in cli/cli#10758 #### New Contributors - [@​leudz](https://github.com/leudz) made their first contribution in cli/cli#10767 **Full Changelog**: cli/cli@v2.70.0...v2.71.0 ### [`v2.70.0`](https://github.com/cli/cli/releases/tag/v2.70.0): GitHub CLI 2.70.0 [Compare Source](cli/cli@v2.69.0...v2.70.0) #### Accessibility This release contains dark shipped changes that are part of a larger GitHub CLI accessibility preview still under development. More information about these will be announced later this month including various channels to work with GitHub and GitHub CLI maintainers on shaping these experiences. ##### Ensure table headers are thematically contrasting [#​8292](cli/cli#8292) is a long time issue where table headers were difficult to see in terminals with light background. Ahead of the aforementioned preview, `v2.70.0` has shipped changes that improve the out-of-the-box experience based on terminal background detection. The following screenshots demonstrate the Mac Terminal using the Basic profile, which responds to user's appearance preferences: <img width="1512" alt="Screenshot of gh repo list in light background terminal" src="https://github.com/user-attachments/assets/87413dde-eec8-43eb-9c16-dc84f8249ddf" /> <img width="1512" alt="Screenshot of gh repo list in dark background terminal" src="https://github.com/user-attachments/assets/7430b42c-7267-402b-b565-a296beb4d5ea" /> For more information including demos from various official distributions, see [#​10649](cli/cli#10649). #### What's Changed ##### ✨ Features - Update go-gh and document available sprig funcs by [@​BagToad](https://github.com/BagToad) in cli/cli#10680 - Introducing experimental support for rendering markdown with customizable, accessible colors by [@​andyfeller](https://github.com/andyfeller) [@​jtmcg](https://github.com/jtmcg) in cli/cli#10680 - Ensure table datetime columns have thematic, customizable muted text by [@​andyfeller](https://github.com/andyfeller) in cli/cli#10709 - Ensure table headers are thematically contrasting by [@​andyfeller](https://github.com/andyfeller) in cli/cli#10649 - Introduce configuration setting for displaying issue and pull request labels in rich truecolor by [@​andyfeller](https://github.com/andyfeller) in cli/cli#10720 - Ensure muted text is thematic and customizable by [@​andyfeller](https://github.com/andyfeller) in cli/cli#10737 - \[gh repo create] Show host name in repo creation prompts by [@​iamazeem](https://github.com/iamazeem) in cli/cli#10516 - Introduce accessible prompter for screen readers (preview) by [@​BagToad](https://github.com/BagToad) in cli/cli#10710 ##### 🐛 Fixes - `run list`: do not fail on organization/enterprise ruleset imposed workflows by [@​BagToad](https://github.com/BagToad) in cli/cli#10660 - Implement safeguard for `gh alias delete` test, prevent wiping out GitHub CLI configuration by [@​andyfeller](https://github.com/andyfeller) in cli/cli#10683 - Pin third party actions to commit sha by [@​BagToad](https://github.com/BagToad) in cli/cli#10731 - Fallback to job run logs when step logs are missing by [@​babakks](https://github.com/babakks) in cli/cli#10740 - \[gh ext] Fix `GitKind` extension directory path by [@​iamazeem](https://github.com/iamazeem) in cli/cli#10609 - Fix job log resolution to skip legacy logs in favour of normal/new ones by [@​babakks](https://github.com/babakks) in cli/cli#10769 ##### 📚 Docs & Chores - `./script/sign` cleanup by [@​iamazeem](https://github.com/iamazeem) in cli/cli#10599 - Fix typos in CONTRIBUTING.md by [@​rylwin](https://github.com/rylwin) in cli/cli#10657 - Improve `gh at verify --help`, document json output by [@​phillmv](https://github.com/phillmv) in cli/cli#10685 - Acceptance test issue/pr create/edit with project by [@​williammartin](https://github.com/williammartin) in cli/cli#10707 - Escape dots in regexp pattern in `README.md` by [@​babakks](https://github.com/babakks) in cli/cli#10742 - Simplify cosign verification example by not using a regex. by [@​kommendorkapten](https://github.com/kommendorkapten) in cli/cli#10759 - Document UNKNOWN STEP in run view by [@​williammartin](https://github.com/williammartin) in cli/cli#10770 #####
Description
Relates to: #10714
While working on #10785 I was frequently getting tripped up by the fact that we don't know which host the feature detector should be targeting, because we don't know the repo that is being targeted. This becomes a lot easier if we host arg parsing up to where it normally lives (PreRun) and give the run what it actually needs (IssueNumbers and a BaseRepo).
It also hoists conditionals up the stack (i.e. is there a base repo or not) so they don't have to be carried around mentally.
This should also free us from passing the detector through in many commands that need no feature detection because they don't use projects, since we can hoist that up too: https://github.com/cli/cli/pull/10785/files. For an example of this see: da1e788#diff-a408a9bdb3a5377b3d4f319497bbdfd42b739de41e6ff3645c2698dde0cc3e8aR129-R132