Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Using db.SearchOrderBy in internal instead of sortType #25808

Open
yp05327 opened this issue Jul 10, 2023 · 1 comment
Open

Using db.SearchOrderBy in internal instead of sortType #25808

yp05327 opened this issue Jul 10, 2023 · 1 comment
Labels
type/proposal The new feature has not been accepted yet but needs to be discussed first.

Comments

@yp05327
Copy link
Contributor

yp05327 commented Jul 10, 2023

Feature Description

As we have a globally db.SearchOrderBy for sorting, in some places we still using xorm.Session.Asc(col_name) and xorm.Session.Desc(col_name) which may make mistakes like #25806 and hard to maintain.

I have made a simple keyword search, and found there are two places is using Asc and Desc directly:

  • models/git/commit_status.go: func sortCommitStatusesSession
  • models/issues/issue_search.go: func applySorts

Screenshots

No response

@yp05327 yp05327 added the type/proposal The new feature has not been accepted yet but needs to be discussed first. label Jul 10, 2023
@CaiCandong
Copy link
Member

I understand what you mean, reuse db.SearchOrderBy as much as possible to avoid bug, but we can only reuse as much as we can, there are still some complex cases where we can't avoid the need to use xorm.Session.Asc(col_name) and xorm.Session.Desc(col_name) directly. For example models/issues/issue_search.go: func applySorts.
So I think your previous modification #25806 was somewhat inappropriate : you change SortType string to OrderBy db.SearchOrderBy ,I don't think SearchOrderBy can cover all cases, and the role of SearchOrderBy should just be to provide some reusable sorting logic。 I'm going to propose a PR to implement it, and I'll probably make some changes to the commit you made in #25806.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
type/proposal The new feature has not been accepted yet but needs to be discussed first.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants