Skip to content

Add label list sorting and remove command - #5503

Merged
samcoe merged 6 commits into
cli:trunkfrom
heaths:issue5489
May 3, 2022
Merged

samcoe merged 6 commits into
cli:trunkfrom
heaths:issue5489

Conversation

@heaths

@heaths heaths commented Apr 22, 2022

Copy link
Copy Markdown
Contributor

Resolves #5489

@heaths

heaths commented Apr 22, 2022

Copy link
Copy Markdown
Contributor Author

This includes much of PR #5452 and I'll rebase onto trunk after that's merged. For now, take a look at https://github.com/cli/cli/pull/5503/files/d24e632f29c0ac12e69cee7c8323c6820a5978f9..HEAD. I figured given time zone difference, I'd give a head start for expediency.

@heaths
heaths marked this pull request as ready for review April 25, 2022 18:29
@heaths
heaths requested a review from a team as a code owner April 25, 2022 18:29
@heaths
heaths requested review from mislav and removed request for a team April 25, 2022 18:29
@cliAutomation cliAutomation added the external pull request originating outside of the CLI core team label Apr 25, 2022
@heaths heaths mentioned this pull request Apr 25, 2022
@samcoe samcoe self-assigned this Apr 26, 2022
@samcoe
samcoe self-requested a review April 26, 2022 05:38
@samcoe

samcoe commented Apr 27, 2022 •

Copy link
Copy Markdown
Contributor

@heaths Thanks for getting this work started. I am still reviewing but I wanted to discuss the platform limitation regarding having both a search query and sort clause. As you suspected these two options are somewhat incompatible. I dove into the implementation and what is happening is that when there is a query phrase the platform uses elasticsearch to perform the search. Elasticsearch does its own scoring based on the search terms used and then will return the results based on best matches according to the score. Elasticsearch also supports sorting these results by created_at date but not by name. I am not sure why that is.

With this information I think we have three options for moving forward with this:

  1. Use your current approach where gh always does the sorting when the query phrase is provided. This works but has two downsides that I can see. First, is that it has no way to leave the labels sorted according to elasticsearch score. Second, is that it does the sorting after the results are returned so there is a possibility that results would have been different than if they were sorted by elasticsearch and then truncated based on the limit.
  2. We allow elasticsearch to do the sorting when "created" is selected and have gh do the sorting when "name" is selected. When "name" is selected this has the same downsides as above where the results are sorted after being returned.
  3. We disallow using the sort/order and search flags together. Personally this is my choice, I think if a user is using --search to query labels they would most often want the results ordered by best match of their query term which is what this would give us. Additionally, this is what the web UI does.

What are your thoughts?

@heaths

heaths commented Apr 27, 2022

Copy link
Copy Markdown
Contributor Author

I like 3 as well. Straight forward and would retain what I imagine is a match-based sort order by default. After all, it is a search, not a filter. I'll go ahead and make those changes.

@heaths

heaths commented Apr 27, 2022

Copy link
Copy Markdown
Contributor Author

Rebased on trunk and removed the client-side sorting.

@samcoe samcoe left a comment

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.

@heaths This is coming along well. I left a handful of comments. Let me know if you have any questions about them. Thanks for the patients on this one.

Comment thread pkg/cmd/label/http.go Outdated
Comment thread pkg/cmd/label/list.go Outdated
Comment thread pkg/cmd/label/list.go Outdated
Comment thread pkg/cmd/label/list.go
Comment thread pkg/cmd/label/list.go Outdated
Comment thread pkg/cmd/label/list.go
Comment thread pkg/cmd/label/remove.go Outdated
Comment thread pkg/cmd/label/remove.go Outdated
Comment thread pkg/cmd/label/remove.go Outdated
Comment thread pkg/cmd/label/shared.go Outdated

@samcoe samcoe left a comment

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.

Thanks for addressing the comments. This is ready to 🚢

Comment thread pkg/cmd/label/delete_test.go
@samcoe
samcoe enabled auto-merge (squash) May 3, 2022 07:27
@samcoe
samcoe merged commit f0027fd into cli:trunk May 3, 2022
@heaths
heaths deleted the issue5489 branch May 3, 2022 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

external pull request originating outside of the CLI core team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improvements to label list

3 participants