Add the ability to edit issues #2432

jfahrer · 2020-01-04T02:42:26Z

This PR adds the ability to edit existing issues using the same interface as hub issue create.

There is a little duplication between creating and editing issues that could be removed. I'm not convinced that removing the duplication would increase clarity and readability.

commands/issue.go

mislav

Awesome job, looks fantastic, thank you! The test coverage is great 🏆 Left some requests for cosmetic changes mostly.

One thing that rubs me the wrong way (but needn't block your PR right now) is the usage scenario for opening the text editor to edit issue title & body:

hub issue edit 1337 --edit

The fact that one has to write "edit" twice feels awkward to me, but I don't know what's a better solution. In hub release edit, we open the text editor by default to edit the existing release title & body; one can skip that by passing --message "", but this is also awkward.

I'm not sure which would be the better way in the long run. Since this is a new feature, perhaps we could consider another API that's even better than both hub issue edit --edit and hub release edit -m ""? However, I won't require code changes from you around this until we reach some consensus.

features/issue.feature

commands/issue.go

features/issue.feature

mislav · 2020-01-06T14:14:32Z

For posterity: ref. #1686 #1523

jfahrer · 2020-01-06T19:46:42Z

I addressed the issues you pointed out. Let me know if there is anything else.

hub issue edit 1337 --edit
The fact that one has to write "edit" twice feels awkward to me, but I don't know what's a better solution.

Agreed. Maybe a --no-edit flag would make more sense here? Then we could open the editor by default and one can still script updating an issue without changing the title/body

I'm not sure which would be the better way in the long run. Since this is a new feature, perhaps we could consider another API that's even better than both hub issue edit --edit and hub release edit -m ""? However, I won't require code changes from you around this until we reach some consensus.

If you have ideas for the API let me know, I'm happy to tackle them.

jfahrer · 2020-01-14T01:11:29Z

@mislav, any thoughts on the PR and API?

mislav · 2020-01-14T21:58:09Z

@jfahrer Sorry, I had to think this over.

Off the top of my head, these are parts of an issue that a person might want to edit:

title (potentially in editor)
body (potentially in editor)
assignees (set of multiple)
labels (set of multiple)
milestone

We want to enable the user to be able to programatically edit one or any combination of these fields. We also want to enable the user to interactively edit title & body by opening their text editor, similarly to their experience with create. The user might also want to unset some of these fields; let's consider those usage scenarios too.

Lets look at some hypothetical edit usage and try to guess what it does:

hub issue edit 123 - I feel that this should open the text editor for editing title & body; i.e. I don't think that the user should pass --edit to opt into that.
hub issue edit 123 --labels foo,bar - Here it feels like the user wants to replace the old labels on an issue with labels "foo", "bar". Automatically opening an interactive text editor for editing title & body here might be disruptive to the user's original goal, so maybe we shouldn't do that?
- hub issue edit 123 --edit --labels foo - Perhaps this is how we can opt-in to editor even if we're changing other fields too?
- hub issue edit --labels "" - Could we enable clearing existing labels by setting them to an empty value?
- hub issue edit --labels "+foo,-bar" - Should we consider supporting selectively adding/removing labels using a special syntax like this?
- hub issue edit --add-labels foo --remove-labels bar - …or like this?
hub issue edit 123 -m "hello" - Here it looks like the user wants to edit the issue title non-interactively. What shuld happen with existing issue body; should it remain the same or be blanked out?
hub issue edit 123 -m "new title" -m "new body" - Set both title and body to new values non-interactively. I think this will work with your current implemenation. 👍

Should we support editing only body while keeping the title intact, and if so, how would we go about that?

Sorry for so many questions. I'm just brainstorming how I envision editing on the command-line to go. I think the reason we didn't support editing so far is because is so much more complex than creating. Anyway, any thoughts that you have are welcome.

jfahrer · 2020-01-15T23:48:16Z

@jfahrer Sorry, I had to think this over.

Thanks for taking the time!

hub issue edit 123 - I feel that this should open the text editor for editing title & body; i.e. I don't think that the user should pass --edit to opt into that.

An alternative could be using a different keyword than edit. Maybe hub issue update 123? In this case, the hub issue update 123 --edit is less weird. If no options are passed, we could just open the issue on github.com

hub issue edit 123 --labels foo,bar - Here it feels like the user wants to replace the old labels on an issue with labels "foo", "bar". Automatically opening an interactive text editor for editing title & body here might be disruptive to the user's original goal, so maybe we shouldn't do that?

Since the editor would contain the current title and body I'm not sure it would be that disruptive. There should however be a way to prevent the editor from opening. I saw --no-edit somewhere else in the API. Overall, I guess this is an argument for keeping the --edit flag tho.

hub issue edit 123 --edit --labels foo - Perhaps this is how we can opt-in to editor even if we're changing other fields too?

It feels a little confusing that the behavior changes based on whether certain flags are passed or not. I feel like my prior comment about changing the keyword is better suited here.

hub issue edit --labels "" - Could we enable clearing existing labels by setting them to an empty value?

hub issue edit --labels "+foo,-bar" - Should we consider supporting selectively adding/removing labels using a special syntax like this?

hub issue edit --add-labels foo --remove-labels bar - …or like this?

I think the --add-labels and --remove-labels would be easier from a users perspective. We can support those alongside --labels. Clearing labels with --labels "" makes sense to me. I think this might work already but I have to test.

hub issue edit 123 -m "hello" - Here it looks like the user wants to edit the issue title non-interactively. What should happen with existing issue body; should it remain the same or be blanked out?

You are right. I wonder if we should support --title and --body as flags instead? And ditch supporting -m for this sub-command.

hub issue edit 123 -m "new title" -m "new body" - Set both title and body to new values non-interactively. I think this will work with your current implementation. 👍

This will currently work

Should we support editing only body while keeping the title intact, and if so, how would we go about that?

I assume you mean interactively? I can't really judge whether this something needed or not. It doesn't seem like a crucial feature tho.

mislav · 2020-01-16T19:38:39Z

An alternative could be using a different keyword than edit. Maybe hub issue update 123? In this case, the hub issue update 123 --edit is less weird.

That's a great idea. Let's go with hub issue update <number> [<flags>]. If no flags are given, the command should fail and suggest that you either need to specify --edit or pass some specific fields to be updated.

If no options are passed, we could just open the issue on github.com

I think this could be surprising, so let's not define a default just yet. We can always choose a good default in the future when it becomes clear how people are using issue update.

Should we support editing only body while keeping the title intact, and if so, how would we go about that?

I assume you mean interactively? I can't really judge whether this something needed or not.

You're right, it's pretty edge-casey so we don't have to worry about that for now.

I think the --add-labels and --remove-labels would be easier from a users perspective. We can support those alongside --labels.

I also think that this would be the most user-friendly, but you can skip implementing add/remove labels for now, and just support --labels replacing the list of labels with new ones. We can see if people ask for granular label add/remove operations, and avoid expanding the scope of your PR for now.

I wonder if we should support --title and --body as flags instead? And ditch supporting -m for this sub-command.

Yeah, -m potentially having title and body combined was not a great design choice for hub. (I've inherited that from git commit.) I'm up for having separate --title and --body, but I think we should still support update -m "..." for symmetry with create.

Thank you for your thoughtful suggestions!

jfahrer · 2020-01-19T01:00:00Z

That's a great idea. Let's go with hub issue update <number> [<flags>]. If no flags are given, the command should fail and suggest that you either need to specify --edit or pass some specific fields to be updated.

Excellent. I updated the PR. I couldn't find anything that returns the number of flags or a list of flags so that the hub issue update command can fail if no flags are provided. Did I miss it? Is there a better way to determine if a flag was provided?
If not, I would add a function to ArgsParser that returns the number of provided flags.

Yeah, -m potentially having title and body combined was not a great design choice for hub. (I've inherited that from git commit.) I'm up for having separate --title and --body, but I think we should still support update -m "..." for symmetry with create.

Sounds good. Let me add --title and --body in a separate PR. I assume it would make sense to add those flags to other commands like issue create and pull-request as well?

I also think that this would be the most user-friendly, but you can skip implementing add/remove labels for now, and just support --labels replacing the list of labels with new ones. We can see if people ask for granular label add/remove operations, and avoid expanding the scope of your PR for now.

I'll skip this for now then.

mislav · 2020-01-19T15:34:03Z

If not, I would add a function to ArgsParser that returns the number of provided flags.

That might be one approach, but instead I would opt for the more explicit approach of checking whether specific flags were passed (in case there will be other update flags in the future that aren't related to any specific field):

if !args.HasReceived("--edit") && !args.HasReceived("--message") && !args.HasReceived("--labels") && ... {
  utils.Check(cmd.UsageError("please specify fields to update"))
}

jfahrer · 2020-01-19T17:23:26Z

That might be one approach, but instead I would opt for the more explicit approach of checking whether specific flags were passed (in case there will be other update flags in the future that aren't related to any specific field):

You got it

jfahrer · 2020-01-19T18:06:58Z

utils/args_parser.go

@@ -183,6 +183,15 @@ func (p *ArgsParser) HasReceived(name string) bool {
 	return found && len(f.values) > 0
 }

+func (p *ArgsParser) HasReceivedOneOf(names []string) bool {


Might be too much to open the ArgsParser API up like this. It felt more natural to put this logic here instead of the Issue. Let me know if you want me to move it.

I think this is too specific for ArgsParser (which I intentionally kept small), so I'd rather that this is inlined in issue.go or encapsulated as a function that takes an ArgsParser instance and a list of flags.

Btw you can have a Go function accept a list of arguments instead of a slice:

func HasReceivedOneOf(names ...string) bool { } // usage: HasReceivedOneOf("foo", "bar")

I think this is too specific for ArgsParser (which I intentionally kept small), so I'd rather that this is inlined in issue.go or encapsulated as a function that takes an ArgsParser instance and a list of flags.

Sounds good, I inlined the check in issue.go.

Btw you can have a Go function accept a list of arguments instead of a slice:

Cool, I didn't think of that. Thanks for the tip!

mislav · 2020-01-20T13:10:57Z

utils/args_parser.go

@@ -183,6 +183,15 @@ func (p *ArgsParser) HasReceived(name string) bool {
 	return found && len(f.values) > 0
 }

+func (p *ArgsParser) HasReceivedOneOf(names []string) bool {


I think this is too specific for ArgsParser (which I intentionally kept small), so I'd rather that this is inlined in issue.go or encapsulated as a function that takes an ArgsParser instance and a list of flags.

Btw you can have a Go function accept a list of arguments instead of a slice:

func HasReceivedOneOf(names ...string) bool { } // usage: HasReceivedOneOf("foo", "bar")

This made sense for `issue create`, where a person would want to obtain the URL to the new issue, but it makes less sense for `issue update`, where the user already knows the issue number (which will not change). This makes it so that the successful run has no output, but since I don't know what a better confirmation behavior would be, I think this is fine for now.

A body will typically have CR-LF when it was authored using web UI on Windows.

mislav · 2020-01-21T11:58:08Z

@jfahrer Pushed some tweaks to your branch, such as supporting updating issue state. Thank you for the amazing work here! ✨

jfahrer · 2020-01-21T17:15:26Z

Thank you!

Add the ability to edit issues

fc2b446

jfahrer commented Jan 4, 2020

View reviewed changes

commands/issue.go Show resolved Hide resolved

mislav requested changes Jan 6, 2020

View reviewed changes

features/issue.feature Outdated Show resolved Hide resolved

features/issue.feature Outdated Show resolved Hide resolved

commands/issue.go Outdated Show resolved Hide resolved

commands/issue.go Show resolved Hide resolved

features/issue.feature Outdated Show resolved Hide resolved

Extract applying common params into functions

ae70177

Use update instead of edit to update issues

5df6b31

Fail when running issue update without specifying fields to update

ab374d9

jfahrer commented Jan 19, 2020

View reviewed changes

mislav requested changes Jan 20, 2020

View reviewed changes

jfahrer and others added 7 commits January 20, 2020 14:10

Inline check for updating flags

8b7c92e

💅

6597192

Compensate for CR-LF line endings when editing issues/releases

153d8ed

A body will typically have CR-LF when it was authored using web UI on Windows.

Avoid fetching issue unless necessary

c175488

Allow nulifying existing issue's labels, assignees, & milestone

b0db79d

Allow updating issue state

f988235

mislav merged commit 57b8337 into mislav:master Jan 21, 2020

mislav mentioned this pull request Jan 21, 2020

Close pull-requests #1151

Closed

mislav mentioned this pull request Jan 21, 2020

Updating a pull request #1523

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the ability to edit issues #2432

Add the ability to edit issues #2432

jfahrer commented Jan 4, 2020 •

edited

Loading

mislav left a comment •

edited

Loading

mislav commented Jan 6, 2020

jfahrer commented Jan 6, 2020 •

edited

Loading

jfahrer commented Jan 14, 2020

mislav commented Jan 14, 2020 •

edited

Loading

jfahrer commented Jan 15, 2020

mislav commented Jan 16, 2020

jfahrer commented Jan 19, 2020

mislav commented Jan 19, 2020

jfahrer commented Jan 19, 2020

jfahrer Jan 19, 2020

mislav Jan 20, 2020

jfahrer Jan 20, 2020

mislav Jan 20, 2020

mislav commented Jan 21, 2020

jfahrer commented Jan 21, 2020

Add the ability to edit issues #2432

Add the ability to edit issues #2432

Conversation

jfahrer commented Jan 4, 2020 • edited Loading

mislav left a comment • edited Loading

Choose a reason for hiding this comment

mislav commented Jan 6, 2020

jfahrer commented Jan 6, 2020 • edited Loading

jfahrer commented Jan 14, 2020

mislav commented Jan 14, 2020 • edited Loading

jfahrer commented Jan 15, 2020

mislav commented Jan 16, 2020

jfahrer commented Jan 19, 2020

mislav commented Jan 19, 2020

jfahrer commented Jan 19, 2020

jfahrer Jan 19, 2020

Choose a reason for hiding this comment

mislav Jan 20, 2020

Choose a reason for hiding this comment

jfahrer Jan 20, 2020

Choose a reason for hiding this comment

mislav Jan 20, 2020

Choose a reason for hiding this comment

mislav commented Jan 21, 2020

jfahrer commented Jan 21, 2020

jfahrer commented Jan 4, 2020 •

edited

Loading

mislav left a comment •

edited

Loading

jfahrer commented Jan 6, 2020 •

edited

Loading

mislav commented Jan 14, 2020 •

edited

Loading