Edit issue command

[samcoe]

This PR implements the issue edit command allowing the ability to update an already created issue. I extracted code that will be used for the pr edit command into the editable file. Overall the command behavior is very similar to issue create and pr create.

closes #904
follow ups #2940 #2949

[mislav]

The code looks pretty solid; great work!

Before I approve, something that I feel needs clarifying: then supplying values via flags, e.g. gh issue edit --label foo --label bar, will this add the foo + bar labels to the set of existing labels, or replace all old labels? From the flags documentation, it sounds like new labels will be added, but from my reading of the code, it sounds like the whole set will get replaced.

Would there be a way to non-interactively explicitly add a label (preserving old ones), remove a specific label, or clear all existing labels?

[mislav]

Instead of passing around funcs for the purpose of stubbing in tests—even though that is the pattern that we practiced before—how about we consider gradually moving to interfaces? It would allow the same kind of dependency injection, with the added benefit of better readability/being more idiomatic Go.

You don't have to rewrite all of these; I'm just wondering whether you think that accepting interfaces and then substituting the concrete implementation object in tests would be an approach that could make the above cleaner.

[samcoe]

I like the interface approach and generally think it is better. I mostly just forgot that we had discussed it when I wrote this and ended up falling back on what we had been doing.

My only real complaint with the interface approach is that it doesn't seem to bring much improvement when the interface defines only one function and results in slightly more code. I do think bundling up the survey functions into a surveyAsker interface, as seen in the gist, is great though.

I will keep the idea top of mind when I do the pr edit command and see how that goes, and if it works well I will come back and update this command.

[samcoe]

@mislav That is a great question about the non-interactive flow. The code does replace the assignee/label/project value completely rather than adding to the existing field and so if we decided to keep this behavior I will update the documentation to reflect that.

If we want to also support adding/removing/clearing values of fields that accept arrays of values we would probably want to introduce new flags such as --add-label, --remove-label, --clear-label. I can't think of a nice way to do this with one flag that does not lead to confusion. I do see value in supporting these sorts of operations, but I am wondering if that can be implemented as a separate feature?

[vilmibm]

Looks awesome, thank you!

For the --label question, I'd love the ability to add labels incrementally or replace wholesale, but I'm fine with it being beyond the scope of this PR. I think for this PR having --label replace wholesale is fine. As a follow up I'd like to explore an --add flag that changes the meaning of the various fields -- ie they all become additive (not just labels).

[mislav]

I do see value in supporting these sorts of operations, but I am wondering if that can be implemented as a separate feature?

Of course, since that adds the complexity of extra flags, it can absolutely be done as follow-up. However, I do see features of adding/removing a label (or assignee, project, etc) as being vastly more useful than replacing the whole set of labels. In fact, I can't really think of a use case where I would want to replace the whole set. On the web, that operation is not even possible. That's why I feel that adding/removing functionality should be a priority over replacing.

I can't think of a nice way to do this with one flag that does not lead to confusion.

Some CLI tools support syntaxes like --flag +foo,+bar,-baz to denote add/remove operations from a set, but I'm not sure if we should go in that direction, especially because operators + and - are themselves valid characters in a label.

[billygriffin]

I do see features of adding/removing a label (or assignee, project, etc) as being vastly more useful than replacing the whole set of labels. In fact, I can't really think of a use case where I would want to replace the whole set. On the web, that operation is not even possible. That's why I feel that adding/removing functionality should be a priority over replacing.

I'm in strong agreement with this. I'd be concerned about us shipping something where adding a label via gh just overwrites all existing labels. I'm curious if @ampinsk has thoughts on how to address this.

[ampinsk]

I'm in strong agreement with this. I'd be concerned about us shipping something where adding a label via gh just overwrites all existing labels. I'm curious if @ampinsk has thoughts on how to address this.

I'm also in agreement here. A default behavior of adding labels is much less destructive and more expected than replacing.

This behavior and syntax is a good question. Separating these behaviors out into --add-label --remove-label at minimum feels simple and intuitive at least as a start? Also agree this could be a follow up.

[mislav]

Looks great! Thank you for the hard work 🏆

[mislav]

Tiny naming oddity: it's a bit weird that simply declaring your intent that you want to edit the title would set the opts.TitleEdited field to true, and that the same field is used when --title was passed on the command-line. But, I guess it works!