Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add flags to add additional metadata to issue/pr create #787

Merged
merged 24 commits into from
May 8, 2020

Conversation

mislav
Copy link
Contributor

@mislav mislav commented Apr 15, 2020

Until we reach a consensus on how we want to interactively choose metadata during issue/pr create #768, this adds support for flags and introduces a rough mechanism for converting assignees/labels/projects/milestone names to their GraphQL ID.

  • -r, --reviewer (PRs only, accepts multiple)
  • -a, --assignee (accepts multiple)
  • -l, --label (accepts multiple)
  • -p, --project (accepts multiple)
  • -m, --milestone

Example:

gh issue create -a vilmibm -l bug -l "help wanted" -p roadmap -m "the big 1.0"

TODO:

  • hide “Preview in browser” when any of these flags are used, since we can't forward those values via query parameters
  • extend these flags to pr create
  • -r, --reviewer (accepts multiple) for pr create
    • support team names in -r, --reviewer
  • tests
  • prefetch more than 100 entries per metadata set (e.g. assignees, labels, etc.)?
  • avoid prefetching repository metadata that isn't needed (e.g. labels when --label wasn't set)

Depends on #786
Ref. #340

- `-a, --assignee` (accepts multiple)
- `-l, --label` (accepts multiple)
- `-p, --project` (accepts multiple)
- `-m, --milestone`
@mislav mislav marked this pull request as ready for review April 27, 2020 17:01
@mislav mislav requested review from vilmibm and probablycorey April 27, 2020 17:01
Copy link
Contributor

@probablycorey probablycorey left a comment

Choose a reason for hiding this comment

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

The code looks good, but I ran into problems running the commands. I get this error:

error fetching organization teams: Your token has not been granted the required scopes to execute this query. The 'id' field requires one of the following scopes: ['read:org', 'read:discussion'], but your token has only been granted the: ['repo'] scopes. Please modify your token's scopes at: https://github.com/settings/tokens.

I thought maybe I was using an old token or something, but I removed `.config/gh/config.yml and I had the same problems with the new token. Do we have to update the app permissions?

v4 := githubv4.NewClient(client.http)

var projects []RepoProject
for {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still not used to golang's "while" loops

if err != nil {
return nil, err
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Since it looks like the reviewQuery and the updateQuery are not dependent on each other, I was wondering if we could combine the queries and variables and just make one call to the server?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting suggestion! That should definitely be possible, even though we don't have a clean mechanism of merging GraphQL queries just yet. I will look into whether this is feasible

return len(vv) == 0
default:
return true
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Very handy!

for _, assigneeLogin := range names {
found := false
for _, u := range m.AssignableUsers {
if strings.EqualFold(assigneeLogin, u.Login) {
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL: case folding in go

@probablycorey
Copy link
Contributor

Ignore my comment about app permissions. I saw The upstream branch of your current branch does not match the name of your current branch is open and realized this PR wasn't in the "needs review" column yet"

@mislav
Copy link
Contributor Author

mislav commented May 4, 2020

The code looks good, but I ran into problems running the commands. I get this error:

This is completely understandable, and I should have stressed this out better. I wrote in the PR body: Depends on #786. Before #786 got merged into master, in order to test this, one should have checked out that branch and went through the steps of re-authorization to gain the extra OAuth scope.

I've now merged master into this branch, so those manual steps are no longer necessary.

Copy link
Contributor

@probablycorey probablycorey left a comment

Choose a reason for hiding this comment

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

Works great!

Copy link
Contributor

@vilmibm vilmibm left a comment

Choose a reason for hiding this comment

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

great work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants