Skip to content

Fix filtering of PR list#50

Merged
vilmibm merged 1 commit intoprototypefrom
filterfix
Nov 6, 2019
Merged

Fix filtering of PR list#50
vilmibm merged 1 commit intoprototypefrom
filterfix

Conversation

@vilmibm
Copy link
Copy Markdown
Contributor

@vilmibm vilmibm commented Nov 5, 2019

unfortunately there is no ALL state for filtering pull requests; you have to specify all of the types (or specify nothing, but we have a default of OPEN).

This PR tweaks the PR filtering to match what graphql ultimately wants -- an array of pull request states.

@vilmibm vilmibm changed the base branch from master to prototype November 5, 2019 21:51
Copy link
Copy Markdown
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.

Good catch! I must have not tested "all" at all. I think I got "all" from the REST API and somehow projected it here.

graphqlState = []string{"MERGED"}
case "all":
graphqlState = "ALL"
graphqlState = []string{"OPEN", "CLOSED", "MERGED"}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Doesn't MERGED also imply CLOSED? I'm wondering if we have to explicitly list it here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

no, merged is a distinct state. closed means the PR was closed without merging.

@vilmibm vilmibm merged commit 7241f4c into prototype Nov 6, 2019
@mislav mislav mentioned this pull request Nov 6, 2019
@mislav mislav deleted the filterfix branch January 10, 2020 12:49
vilmibm pushed a commit that referenced this pull request Jun 29, 2021
Changed parsefileArg to return a string, reformatted testing, polishe…
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.

2 participants