Skip to content

add search feature in listing issues#3196

Merged
mislav merged 4 commits intocli:trunkfrom
g14a:feature/search-issues
Mar 23, 2021
Merged

add search feature in listing issues#3196
mislav merged 4 commits intocli:trunkfrom
g14a:feature/search-issues

Conversation

@g14a
Copy link
Contributor

@g14a g14a commented Mar 11, 2021

This commit adds a --search flag to gh issues list which uses the Search API. This isn't final. Just an initial trial on the implementation, needs changes and is open to feedback.

Implements a filtering option in #641

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.

Looks like a nice start! Can --search be combined with other flags, like --label or --assignee? If it can, then shouldn't we add those search clauses to the search query?

Can --search be combined with --web? I do not see why not, but some code will have to be moved around for that.

@g14a
Copy link
Contributor Author

g14a commented Mar 12, 2021

@mislav Would --label and --assignee be necessary once we introduce --search though? Because with --search the user can access all the filters relevant to issues right? I was thinking it'd be better if all the search elements are in one place i.e one flag rather than some in --search and one of them in the other. What do you think?

Oh yes --search can be integrated with --web and that's yet to be done.

@mislav
Copy link
Contributor

mislav commented Mar 12, 2021

@g14a Well, unless otherwise stated, I think that different flags on the same command should generally combine well with each other. For example, if I'm already familiar with gh issue list -l bug -a user syntax, and I just want to add a search term to that command, it would be great if gh issue list -l bug -a user --search "foo" just worked without me having to rewrite the whole query as `gh issue list --search "label:bug assignee:user foo".

@g14a
Copy link
Contributor Author

g14a commented Mar 12, 2021

@mislav That's actually a good idea to not force the user to use a different flag all of a sudden and forget his older familiarity. Okay so you propose we combine the results of multiple flags and present them as one whole result set. Correct me if I'm wrong?

@g14a
Copy link
Contributor Author

g14a commented Mar 14, 2021

@mislav I've combined the rest of the flags with --search i.e all flags with keep appending to a search query which finally gets executed. I think the IssueList function can be removed as wouldn't be used in gh issue list after this commit. Also we needn't have Milestone by number and Milestone by title as search does all we need.

Search has been combined with --web as well.

I'd like some feedback on this :)

@23leliana32

This comment has been minimized.

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.

Thanks, this is going in a great direction.

I'm wondering if search query generation can be reused instead of re-implemented and if we can keep using IssueList() in non-search scenarios. Detailed thoughts below

@g14a
Copy link
Contributor Author

g14a commented Mar 17, 2021

@mislav I've tried to improve the PR with your feedback and wrote new tests separately for --search with respect to the backwards-compatibility that you mentioned. Hope I didn't make things worse.

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.

This looks so much better! Only minor polish pieces remaining

@g14a
Copy link
Contributor Author

g14a commented Mar 18, 2021

@mislav I've completed the minor polish pieces. Thanks for the assistance!

@mislav mislav force-pushed the feature/search-issues branch from d0b081f to 34680a5 Compare March 19, 2021 12:43
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. I've pushed some tweaks and a major restructuring of the tests, but overall, this works great!

@mislav mislav requested a review from a team March 19, 2021 12:56
Copy link
Contributor

@samcoe samcoe left a comment

Choose a reason for hiding this comment

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

Left one naming comment, but overall this looks great! Nice job!

@g14a g14a requested a review from mislav March 23, 2021 14:43
@mislav mislav force-pushed the feature/search-issues branch from 174d40b to ef93ba2 Compare March 23, 2021 17:11
@mislav mislav force-pushed the feature/search-issues branch from ef93ba2 to 179d3f0 Compare March 23, 2021 17:53
@mislav mislav merged commit 9b0f706 into cli:trunk Mar 23, 2021
@mislav
Copy link
Contributor

mislav commented Mar 24, 2021

Thank you @g14a for all the hard work!

@ghost

This comment was marked as spam.

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.

4 participants