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 filters issues #644

Merged
merged 7 commits into from
Jul 2, 2020
Merged

Add filters issues #644

merged 7 commits into from
Jul 2, 2020

Conversation

eddumelendez
Copy link
Contributor

This commit introduces two new flags in order to filter issues per mentioned and milestone.

See #641

@@ -171,7 +171,7 @@ func IssueStatus(client *Client, repo ghrepo.Interface, currentUsername string)
return &payload, nil
}

func IssueList(client *Client, repo ghrepo.Interface, state string, labels []string, assigneeString string, limit int, authorString string) (*IssuesAndTotalCount, error) {
func IssueList(client *Client, repo ghrepo.Interface, state string, labels []string, assigneeString string, limit int, authorString string, mentionedString string, milestoneString string) (*IssuesAndTotalCount, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't want to sound picky but don't you guys think it's a little redundant to have the String suffix in the arg name?
Like in mentionedString, assigneeString

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed that it's starting to look weird, but I don't think this is something that we need to address in this PR. As a follow-up, we will look into having IssueList accept a struct of filtering options or something similar, rather than a big argument list that starting to be hard to maintain.

@eddumelendez Thank you for this PR! The code looks good; we're just holding off merging features in general until we figure out things like exact flag naming and how we can fit these features in our release schedule ✨

@vilmibm vilmibm self-requested a review April 29, 2020 01:13
@BondAnthony
Copy link

Just a question, but how would one retrieve the milestone number to use in this command? I was hoping to pass the milestone title but that doesn't appear to work based on the docs. The milestone number also doesn't work. It looks like one would need to decode the ID and use that number. Please tell me I'm looking at this wrong.

@mislav mislav changed the base branch from master to trunk May 27, 2020 11:41
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.

@eddumelendez I tested this out locally but got this error:

graphql error: 'Variable $mentioned is used by but not declared, Variable $milestone is used by but not declared'

I think it is because the milestone and mentioned vars need to be defined in the query here https://github.com/cli/cli/pull/644/files#diff-f9ae6189655de6363f9ac71892daec02R215.

@mislav once this works do you think we can move forward with it, or do you still want to figure out the flag naming scheme you mentioned in https://github.com/cli/cli/pull/644/files#r395592397

@eddumelendez
Copy link
Contributor Author

thank you so much @probablycorey good catch!

I have rebased the branch and add the fix

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.

@probablycorey This looks good to me! One tiny nit about the flag name though

command/issue.go Outdated
@@ -41,6 +41,8 @@ func init() {
issueListCmd.Flags().StringP("state", "s", "open", "Filter by state: {open|closed|all}")
issueListCmd.Flags().IntP("limit", "L", 30, "Maximum number of issues to fetch")
issueListCmd.Flags().StringP("author", "A", "", "Filter by author")
issueListCmd.Flags().StringP("mentioned", "", "", "Filter by mention")
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 not 100% sold on "mentioned" since it's past tense. How about present tense, e.g. --mentions ampinsk? @ampinsk: would love to know your thougths.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just --mention?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the flag name

@mislav mislav dismissed probablycorey’s stale review June 30, 2020 12:30

addressed GraphQL error, flag naming

@mislav mislav merged commit ed78110 into cli:trunk Jul 2, 2020
@@ -212,10 +212,10 @@ func IssueList(client *Client, repo ghrepo.Interface, state string, labels []str
}

query := fragments + `
query($owner: String!, $repo: String!, $limit: Int, $endCursor: String, $states: [IssueState!] = OPEN, $labels: [String!], $assignee: String, $author: String) {
query($owner: String!, $repo: String!, $limit: Int, $endCursor: String, $states: [IssueState!] = OPEN, $labels: [String!], $assignee: String, $author: String, $mention: String, $milestone: String) {
repository(owner: $owner, name: $repo) {

Choose a reason for hiding this comment

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

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.

7 participants