Edit multiple issues, PRs in parallel

[heaths]

Resolves #5782 by allowing multiple issues or PRs to be edited in parallel, and querying for shared fields once to reduce network requests.

[heaths]

This is not complete, but I was hoping for an early review to see if you're good with this approach. It's similar to some other CLI commands and what I've done in some GH extensions.

It allows interactive with only 1 issue per discussion in #5782. Since I check early as part of argument validation, the editRun function is roughly the same, with a couple of interactive checks to prompt. That would only actually happen if there was 1 issue, so the logic is left as-is and simple.

There's still a few things to do:

• Do the same for gh pr edit (will do this in a separate PR)
• Reorder or refactor how shared editable fields are fetched since the defaults are no longer set. In manual testing, I found that this breaks the flow e.g., selecting "Assignees" does not show anyone or allow you to specify someone and instead just asked to submit, prompting for Y/n.
• Add tests for issue's and PR's editRun functions.

[heaths]

@mislav @samcoe what do you think of the direction here? I'll make similar changes to pr edit and adds tests for multiple issues for both.

I refactored the brunt of the lookup code into separate methods because so many commands call the singular version, so it seemed like a good idea to consolidate all the checks therein; though, part of me feels that may be unnecessary in an EXE project because those commands only work with a single issue anyway. Perhaps a separate method could just panic on completely unexpected results e.g., multiple outputs, which shouldn't happen. Sort of like runtime asserts you should never hit anyway. Thoughts?

[samcoe]

@heaths Thanks for getting start on this. Left a handful of inline code comments. Let me know if you have any questions about them.

[heaths]

@samcoe I'm done with runtime changes and manual testing but having problems with writing tests. What's used in cli/cli is rather limited, making it difficult to identify specific requests unlike gock used in cli/go-gh. For example, I need to return an error from an updateIssue to test the success + failure collection code. Seems the Matcher isn't capable, though I haven't fully explored what all other code is potentially doing here. May need to write my own matcher. Or, what about introducing gock into cli/cli?

Updated: I added a matcher for mutations - could do the same for queries, but didn't need it so it'd get no coverage - in lieu of gock and this allows me to test the failure scenarios for both issue queries and updates.

If you're good with this pattern, I'll replicate the same for PRs. Basically, I sort and output all the successful issue URLs first and then the failed URLs since the CLI would return a error as the final line: it'd be all grouped together.

Alternatively, I considered aggregating all errors into a single one, but the CLI currently targets Go 1.19 and not 1.20 so there's no built-in error wrapping. I'm open to changing that for this PR, though.

[samcoe]

This is looking real good, almost there I think! Appreciate your patients going through this review process.

[heaths]

@samcoe as far as issues go, this should be ready. I rebased on trunk but it may take a little more time to make the changes to PRs. When I first looked at this, I took a cursory glance and it seemed that, as with some other places in the CLI, issues and PRs shared more code than they actual do once I started digging in.

On that note, would you consider taking issues first? And should I refactor PRs or issues to actually share more? Issues have the function we'd be discussing while PRs uses a Finder. PRs also don't prompt for what to edit, but I don't intend to change that unless you wanted me to. I'm trying to keep refactoring to a minimum to reduce risk, so for now I'll continue with such minimal changes unless you want me to unify the code more.

[samcoe]

@heaths This looks great to me! Thanks for the hard work on this.

I don't see a problem with merging/releasing this before pr edit gets changed. I don't think they need to be shipped together, each feature provides value on its own.

For pr edit I think keeping the change set to a minimum is the right approach and we shouldn't try to refactor the two commands to share more code unless it really makes sense. As you said, the two commands have some differing behavior so I think it is completely fine if we end up with some duplicate code.

[heaths]

@samcoe I've got a few ideas of how to support this for PRs while keeping changes to a minimum. Almost all PR commands use the Finder and Finder.Find would be a lot to duplicate as I did with IssuesFromArgsWithFields, but like that function simply calling one within another doesn't work ideally. What if, instead, I do what I did originally with IssuesFromArgsWithFields and had a singular function that calls the plural variant and either assumes or asserts a single PR is returned (probably safe to assume)? Finder.Find is already using errgroup so adding another goroutine seems insignificant.

The idea would be that no selectors means the current branch as it is currently, but then condition into a loop of 1 or more PR numbers, URLs, or branch names. Find could be a wrapper that assumes a single issue is returned, while MultiFind ("multi" seems to be a common enough prefix in the codebase, but I'm open to other suggestions, of course) can return multiple. That was initially the goal of IssuesFromArgsWithFields but I appreciate it wasn't using errgroup or even a wait group before, but Find already does.

If you'd prefer - I know we overlap a bit in your afternoon / my morning - perhaps we could chat over Teams.

[samcoe]

@heaths Agreed that changing finder.Find would cause too much of a wide spreading change and that adding a new function is the the better approach. I am partial to the name finder.FindMultiple but I don't have strong opinions on it. This overall approach seems good to me.

[heaths]

@samcoe I more or less started down that path, though with a different name. I like yours better and will change it.

The only widespread change I ended up doing was making FindOptions.Selector and BaseBranch []string and renaming them plurally. The single + multiple fields just felt so wrong I couldn't bring myself to do it. Sure, it touched ~10 files but it's all 1- or 2-liners. I'll continue with that for a separate PR.

[mislav]

Thanks for the hard work on this @heaths and @samcoe for stewarding this! 🚀

[mislav]

Due to the async nature of this function's implementation, the resulting slice of api.Issue results does not necessarily match the order of input args. It doesn't seem like that negatively affects the rest of the edit implementation, but I wonder will this be apparent to future callers of this function. Just a thought!

[heaths]

Order shouldn't matter, true, and since it didn't even handle multiple issues before I don't foresee any regression. That said, @samcoe and I did briefly discuss whether to just lexically sort the issues or try to match the order in which they were specified - the latter of which didn't seem necessary. Could still be done, though. I would hope - and see on reason - callers would care about the order specified.