-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
Added language as a filter to explore/repo_search #24212
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was already working on this and almost finished, but you were faster. I have left a few comments with the toughs I was facing during developing this.
Any screenshot? |
Sure! |
I will send my LGTM now. But a further step is to use async menus with a search box because the programming languages may be very big on public websites. |
I think this should be done, before this is merged. |
Just added search |
Please define "big". The number after which to go async depends on the dropdown performance. I have other UIs where I don't consider pagination until 10k items. Test first how many items the dropdown can render performantly, and then decide whether pagination is worth it. How big can the language list get? Maybe 200? |
Well, I don't think I have the resources to test on even a moderately sized instance. However initially, rendering |
@JakobDev looking good? |
The API needs a little bit of improvement. If you want to get all languages for a User you need o load all Repos of that User and pass the IDs to We need also tests for the API to make sure it keeps working as expected. If you don't know how to write tests, I can do that for you. |
models/repo/repo_list.go
Outdated
func GetPrimaryRepoLanguageList(ctx context.Context, repos RepositoryList) (LanguageStatList, error) { | ||
languageList := make(LanguageStatList, 0) | ||
|
||
q := db.GetEngine(ctx). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this will perform well in large instances.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed at #24212 (comment)
Unsure what the ideal alternative would be
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a new table to store the languages and counts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a new table to store the languages and counts
Hmm, might work for the global list but unsure how to filter them by the repo owner since a few databases do not support storing primitive types as an array. Might be possible to create yet another table containing a the list of owners but tbf I wonder if the benefits would be worth it at that point
@JakobDev I have added a query param for the ownerId (ignored when repo ids are provided fn) ... felt it was easier than adding multiple api routes :P
Thank you! I have added in a minimal test fn |
You should really add the routes I mentioned instead of using a query param. that would match the behaviour of the other routes together with a test for each. That's mostly copy and paste.
You should also check if the values are correct to make sure this doesn't break in the future. |
@snematoda Can you send the PR again? I want to resolve the conflicts but made the PR broken. |
Added a new language filter beside the existing sort for repos in explore