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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix --milestone flag on issues list command #1462

Merged
merged 11 commits into from
Aug 19, 2020
Merged

Conversation

fsmiamoto
Copy link
Contributor

@fsmiamoto fsmiamoto commented Jul 31, 2020

This PR adds a fix to the --milestone flag on the issues list command as described in #1441

$ cli issue list --milestone "AWS Migration"

Showing 1 of 1 issue in Safecast/safecastapi that matches your search

#89  Need How To restore from backup    about 1 year ago

I'm not sure if the approach I took was the best one so any feedback would be great 馃憤

Edit:
Closes #1441

This fix involves using the REST API since the GraphQL one doesn't
seem to return the necessary ID for the subsequent issues query.
@fsmiamoto
Copy link
Contributor Author

The use of the REST API appears to be unavoidable since the GraphQL API does not provide the necessary ID for the use in the query.

Hopefully this could change in the future to avoid this workaround.

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.

Excellent; thanks for tackling this!

api/queries_issue.go Outdated Show resolved Hide resolved
command/issue_test.go Outdated Show resolved Hide resolved
api/queries_issue.go Outdated Show resolved Hide resolved
Since we can obtain the necessary ID for the query on
node ID from the GraphQL API, it makes sense to remove
the REST version.
@fsmiamoto
Copy link
Contributor Author

@mislav
Thanks for the review, I think I've managed to implement the necessary changes

@fsmiamoto fsmiamoto requested a review from mislav August 2, 2020 03:05
@tierninho
Copy link
Contributor

Thanks for the PR! I played around with this and here are some findings:

  1. There should be some type of "no match" result if the following is run gh issue list --milestone "sdhfsdhjf".

    Currently it returns this:

    Showing 5 of 5 issues in tierninho/Blah that match your search
    
    #12  sdfsdf                          about 1 minute ago
    #10  sdfsdfsdfssfdssdf  (duplicate)  about 1 minute ago
    #5   sdfsdf                          about 6 months ago
    #4   My issue title                  about 1 minute ago
    #3   My issue title                  about 8 months ago
    

    it instead should show something like:

    No milestones match your search in tierninho/Blah

  2. The help menu should be edited from:
    -m, --milestone name Filter by milestone name
    to
    -m, --milestone string Filter by milestone

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.

Almost there! One small but important change needed before we would merge this.

api/queries_issue.go Outdated Show resolved Hide resolved
@fsmiamoto
Copy link
Contributor Author

@tierninho @mislav
Thanks for comments!

I've added the check for a not found milestone, alongside with a test, and it should solve the first issue pointed out by @tierninho.

For the second one, it looks like the pattern of using name is spread through out the project so I suggest we make no changes.

@fsmiamoto fsmiamoto requested a review from mislav August 6, 2020 00:19
@tierninho
Copy link
Contributor

Thanks for adding below @fsmiamoto

no milestone found with title: "sdhfsdhjf"

One nitpick though: please wrap it with two empty lines like the pic, as it will match current messaging. Appreciate it!

Screen Shot 2020-08-05 at 3 26 42 PM

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 so much!

@fsmiamoto
Copy link
Contributor Author

Thanks for testing it out @tierninho 馃憤

As for the formatting, since we raise an error for not finding the milestone, I've tried to keep to stick to the existing pattern.

I definitely agree that it looks better on the second command but since it doesn't actually raise an error, the output is handled differently.

@vilmibm vilmibm added this to Backlog 馃棐 in The GitHub CLI via automation Aug 19, 2020
@vilmibm vilmibm merged commit e441ea6 into cli:trunk Aug 19, 2020
The GitHub CLI automation moved this from Backlog 馃棐 to Pending Release 馃 Aug 19, 2020
@matschaffer
Copy link
Contributor

That looks great! Nice work @fsmiamoto - love that http://github.com/safecast got an implicit mention in all this 馃槅

@github-actions github-actions bot moved this from Pending Release 馃 to Done 馃挙 in The GitHub CLI Sep 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
The GitHub CLI
  
Done 馃挙
Development

Successfully merging this pull request may close these issues.

Issues milestone filter does not work with milestone title
5 participants