Skip to content

Fix --milestone flag on issues list command#1462

Merged
vilmibm merged 11 commits intocli:trunkfrom
fsmiamoto:fix-milestone
Aug 19, 2020
Merged

Fix --milestone flag on issues list command#1462
vilmibm merged 11 commits intocli:trunkfrom
fsmiamoto:fix-milestone

Conversation

@fsmiamoto
Copy link
Contributor

@fsmiamoto fsmiamoto commented Jul 31, 2020 •
edited
Loading

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

fsmiamoto added 2 commits July 30, 2020 22:11
@fsmiamoto
fix searching issues by the milestone name
3ce1de6 
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
register mocks for the new request
75e8cb8
@fsmiamoto
Copy link
Contributor Author

fsmiamoto commented Jul 31, 2020

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.

mislav
mislav suggested changes Jul 31, 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.

Excellent; thanks for tackling this!

@fsmiamoto
use graphql for repo milestones on issue list
9a5e69a 
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
add mock for milestones on issue list
f9b50ac
@fsmiamoto
improve comments
3caa7cc
@fsmiamoto
change comment to refer to pr
0faf52d
@fsmiamoto
Copy link
Contributor Author

fsmiamoto commented Aug 2, 2020

@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

tierninho commented Aug 5, 2020

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

mislav
mislav suggested changes Aug 5, 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.

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

@fsmiamoto
Copy link
Contributor Author

fsmiamoto commented Aug 6, 2020

@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

tierninho commented Aug 6, 2020

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

mislav
mislav approved these changes Aug 6, 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.

Fantastic. Thank you so much!

@fsmiamoto
Copy link
Contributor Author

fsmiamoto commented Aug 7, 2020

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 merged commit e441ea6 into cli:trunk Aug 19, 2020
@matschaffer
Copy link
Contributor

matschaffer commented Aug 20, 2020

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

@mislav mislav mentioned this pull request Jan 14, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@mislav mislav mislav approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Issues milestone filter does not work with milestone title

5 participants

@fsmiamoto @tierninho @matschaffer @mislav @vilmibm