Skip to content

issue create: Add special @me assignee (self)#2004

Merged
mislav merged 4 commits intocli:trunkfrom
marclop:f/add-self-assign-flag-to-issue-create
Jan 22, 2021
Merged

issue create: Add special @me assignee (self)#2004
mislav merged 4 commits intocli:trunkfrom
marclop:f/add-self-assign-flag-to-issue-create

Conversation

@marclop
Copy link
Contributor

@marclop marclop commented Sep 28, 2020 •
edited
Loading

Description

This patch adds a new special handling of the @me string when it is
passed as an assignee, to resolve to the currently authenticated user
login which is then resolved to ID using the already existing flow.

Related issues

Closes #965

@marclop
Copy link
Contributor Author

marclop commented Sep 28, 2020

This differs from the proposed --assign-yourself flag name, but it's easy enough to change if you deem it appropriate.

Kamarulzamansalehudin
Kamarulzamansalehudin reviewed Sep 28, 2020
View reviewed changes
Copy link

@Kamarulzamansalehudin Kamarulzamansalehudin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marclop
Copy link
Contributor Author

marclop commented Sep 28, 2020 •
edited
Loading

Once we agree on the flag name I can easily add this same flag to the pr create command.

@mislav
Copy link
Contributor

mislav commented Sep 29, 2020 •
edited
Loading

@marclop What do you think of supporting a special assignee name @me that resolves to the current user's name? That way we could reuse existing flags, e.g. issue create -a @me

GitHub web already supports @me in certain areas like issue search, so maybe we could emulate that and eventually support it as input across different commands, e.g. gh issue list -a @me. How would this feel?

@marclop
Copy link
Contributor Author

marclop commented Sep 29, 2020

@mislav That sounds fine to me, specially if it's how the @me mention is going to align across the board throughout the gh CLI and the GitHub Web UI.-

If so, would we simply treat @me as call via the GraphQL API to api.CurrentLoginName? It'd differ to the general tagging from a "normal" user reference so it feels like a good approach.

@vilmibm vilmibm removed the community label Sep 29, 2020
@mislav
Copy link
Contributor

mislav commented Sep 29, 2020

If so, would we simply treat @me as call via the GraphQL API to api.CurrentLoginName?

Yes that was my idea. Would you like to change this PR to go into that direction instead? It would just need to cover the assign functionality in issue create; we can later expand the approach to other commands.

@vilmibm vilmibm changed the title issue create: Add --self-assign flag support @me for use in assigning things Sep 29, 2020
@vilmibm vilmibm added core This issue is not accepting PRs from outside contributors enhancement a request to improve CLI labels Sep 29, 2020
@vilmibm vilmibm changed the title support @me for use in assigning things issue create: Add --self-assign flag Sep 29, 2020
@vilmibm vilmibm removed core This issue is not accepting PRs from outside contributors enhancement a request to improve CLI labels Sep 29, 2020
@vilmibm
Copy link
Contributor

vilmibm commented Sep 29, 2020

Sorry for the metadata spam! I'm dizzy with issue triage and mistook this for an issue not a PR 🤦‍♀️

@marclop
Copy link
Contributor Author

marclop commented Sep 30, 2020 •
edited
Loading

@mislav yep, I'll change the title, description and code when I have the chance.

@marclop marclop changed the title issue create: Add --self-assign flag issue create: Add special @me assignee (self) Sep 30, 2020
@marclop
Copy link
Contributor Author

marclop commented Sep 30, 2020

d452a8c should implement what we discussed

@matschaffer
Copy link
Contributor

matschaffer commented Oct 6, 2020

@me works for @me! Thanks for picking this up @marclop 🧡

@marclop
Copy link
Contributor Author

marclop commented Oct 7, 2020

@mislav mind a review when you've got time? Thanks!

@vilmibm vilmibm mentioned this pull request Oct 7, 2020
Closed
FeliciaXmL
FeliciaXmL suggested changes Oct 10, 2020
View reviewed changes
Copy link

@FeliciaXmL FeliciaXmL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#8 footer

@marclop
Copy link
Contributor Author

marclop commented Oct 14, 2020

👋 Anyone keen to review this so we can merge it? @mislav or @vilmibm ?

mislav
mislav suggested changes Oct 15, 2020
View reviewed changes
Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late review! This looks great for the non-web flow of creating issues/PRs ❤️

However, it looks to me that the web flow for creating issues/PRs would not resolve @me to the username. The web flow is triggered by either:

  1. Using the gh issue/pr create --web flag;
  2. Using the "Continue in browser" option instead of "Submit" in the interactive create flow.

@marclop
Copy link
Contributor Author

marclop commented Oct 15, 2020

@mislav I missed that code path, I'll take a look tomorrow and fix it.

@marclop
Copy link
Contributor Author

marclop commented Oct 16, 2020

@mislav I've written some tests for the flows:

  • gh issue create -a @me -t <title> -b <body>: Works nicely.
  • gh issue create -a @me (Interactive, select Submit case): Works nicely with submit case no metadata.
  • gh issue create --web -a @me, no problem here if there's no issue templates on the repository, if there are, the user is left to choose one and it renders the ?assignees= query param invalid.

There remains a problematic case where the user prompts interactive creation of the issue, but specifies assignees: gh issue create -a @me. We need to make some decisions on this flow, because as it is, there's some issues with it and how the -a @me needs to be handled:

since I have no permissions to triage issues in cli/cli, I'm using my fork

Adding metadata to the issue when -a is specified

When -a @me for any other assignee is specified in the issue but the user goes into Add metadata, the assignees will get overwritten by whatever the user chooses (or if the user doesn't add them).

$ ./bin/gh issue create -a @me -R marclop/cli

Creating issue in marclop/cli

? Title test
? Body <Received>
? What's next?  [Use arrows to move, type to filter]
  Submit
> Add metadata
  Cancel

$ ./bin/gh issue create -a @me -R marclop/cli

Creating issue in marclop/cli

? Title test
? Body <Received>
? What's next? Add metadata
? What would you like to add?  [Use arrows to move, space to select, <right> to all, <left> to none, type to filter]
> [ ]  Assignees
  [ ]  Labels
  [ ]  Projects
  [ ]  Milestone

To solve this we need to decide:

  • Whether we're ignoring -a when interactive is set and the Add Metadata is selected
  • We're making the user select the assignees directly skipping other fields, appending the current -a ....

Let me know which route I should go and I can work towards that.

@marclop
Copy link
Contributor Author

marclop commented Nov 26, 2020

@mislav Hello? Anybody in the house?

@mislav
Copy link
Contributor

mislav commented Dec 1, 2020

  • Whether we're ignoring -a when interactive is set and the Add Metadata is selected

I think this might get addressed in #2472

Thank you for all the work so far, and sorry for the late review! The ball is in our court now; so please bear with us. 🙇

@marclop marclop force-pushed the f/add-self-assign-flag-to-issue-create branch from a7545c1 to 66f5c06 Compare December 8, 2020 08:11
@mislav mislav force-pushed the f/add-self-assign-flag-to-issue-create branch from 66f5c06 to 73da25a Compare January 22, 2021 21:05
mislav
mislav approved these changes Jan 22, 2021
View reviewed changes
Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic, thank you for all your help!

I've pushed updates to make @me available when listing issues as well, so that support for it is consistent across the board. (It was already supported in gh pr list.)

@mislav mislav merged commit 77cc378 into cli:trunk Jan 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@Kamarulzamansalehudin Kamarulzamansalehudin Kamarulzamansalehudin left review comments

@FeliciaXmL FeliciaXmL FeliciaXmL requested changes

@mislav mislav mislav approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mislav mislav

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Create self-assigned issue

7 participants

@marclop @mislav @vilmibm @matschaffer @FeliciaXmL @Kamarulzamansalehudin @samcoe