[Refactor] Unify repo search order by logic #30876
Conversation
6543
added
the
type/refactoring
GiteaBot
added
the
lgtm/need 2
pull-request-size
bot
added
the
size/L
| // SearchOrderByFlatMap is similar to SearchOrderByMap but use human language keywords | ||
| // to decide between asc and desc | ||
| var SearchOrderByFlatMap = map[string]db.SearchOrderBy{ | ||
| "newest": SearchOrderByMap["desc"]["created"], |
There was a problem hiding this comment.
this make it easyer to match human keywords with api options ... but if prefered I could also use the const directly ...
There was a problem hiding this comment.
but if prefered I could also use the const directly ...
I also prefer. No need to introduce a one-time const or one-time variable ...........
github-actions
bot
added
the
modifies/api
|
I would say this refactor is ready ;) |
I will review and merge after v1.22 stable is released since it's a refactor PR. |
|
@lunny we now have v1.22.0 tagged :) |
GiteaBot
added
lgtm/need 1
| orderBy = db.SearchOrderByForks | ||
| default: | ||
| ctx.Data["SortType"] = "recentupdate" | ||
| if order, ok := repo_model.SearchOrderByFlatMap[sortOrder]; ok { |
There was a problem hiding this comment.
One potential problem with this approach:
ASc or ASC results in the default sorting, even if a valid sorting is given
We should probably lowercase the order first
| // Strings for sorting result | ||
| const ( | ||
| // only used for repos | ||
| SearchOrderByAlphabetically db.SearchOrderBy = "owner_name ASC, name ASC" |
There was a problem hiding this comment.
If it wasn't already present in the original code, I'd propose dropping the Search- prefix in this PR.
It only confuses without bringing any additional value.
As it exists already, I'll see that I open another refactoring PR once this one was merged.
GiteaBot
added
lgtm/done
|
@delvh that are two valid refactors :) But as this is already taking that long i propose to make two small followup pulls adressing it :) |
|
The first one is a bug, not a refactor. |
| // Strings for sorting result | ||
| const ( | ||
| // only used for repos | ||
| SearchOrderByAlphabetically db.SearchOrderBy = "owner_name ASC, name ASC" |
There was a problem hiding this comment.
Every const here is only used once for constructing the "map"
I think all of these const should be removed, and just fill the values into the map directly.
have repo OrderBy definitions defined in one place and use a single type for OrderBy database options