Skip to content
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

replaced query.Sorting with query.Sorts #20723

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Its-Maniaco
Copy link

Fixes #19569

Replaced deprecated query.Sorting with query.Sorts in src/pkg/scan/dao/scanner/registration.go

@Its-Maniaco
Copy link
Author

@stonezdj @zyyw Can you please re-review?
I have made changes based on last suggestion from here.
Apologies for the delay.
My last PR was closed due to inactivity, so reopened a new one

Copy link

codecov bot commented Jul 10, 2024

Codecov Report

Attention: Patch coverage is 10.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 66.23%. Comparing base (c8c11b4) to head (484af6f).
Report is 240 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main   #20723       +/-   ##
===========================================
+ Coverage   45.36%   66.23%   +20.86%     
===========================================
  Files         244     1045      +801     
  Lines       13333   113508   +100175     
  Branches     2719     2845      +126     
===========================================
+ Hits         6049    75178    +69129     
- Misses       6983    34222    +27239     
- Partials      301     4108     +3807     
Flag Coverage Δ
unittests 66.23% <10.00%> (+20.86%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/pkg/scan/dao/scanner/registration.go 56.25% <10.00%> (ø)

... and 1286 files with indirect coverage changes

@@ -123,10 +123,18 @@ func ListRegistrations(ctx context.Context, query *q.Query) ([]*Registration, er
}

// Order the list
if query.Sorting != "" {
qs = qs.OrderBy(query.Sorting)
if len(query.Sorts) > 0 {
Copy link
Member

@chlins chlins Jul 12, 2024

Choose a reason for hiding this comment

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

I think the following logic is duplicated because when call the orm.QuerySetter, there will be a common setSorts function to handle it.

func setSorts(qs orm.QuerySeter, query *q.Query, meta *metadata) orm.QuerySeter {

Copy link
Author

@Its-Maniaco Its-Maniaco Jul 12, 2024

Choose a reason for hiding this comment

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

So, the Query Setter is already returning in sorted order and we just have to remove the explicit Sorting from the code?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, since there is a common logic processing, there is no need for redundant processing here.

@wy65701436 wy65701436 assigned wy65701436 and unassigned stonezdj Jul 15, 2024
if query.Sorting != "" {
qs = qs.OrderBy(query.Sorting)
} else {
qs = qs.OrderBy("-is_default", "-create_time")
Copy link
Member

Choose a reason for hiding this comment

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

Please check if we need to keep this line to ensure backward compatibility without introducing inconsistent results from previous versions, because the common query logic processor will not handle this special case.

Copy link

This PR is being marked stale due to a period of inactivty. If this PR is still relevant, please comment or remove the stale label. Otherwise, this PR will close in 30 days.

@github-actions github-actions bot added the Stale label Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Removing Query.Sorting as it we are deprecating it
7 participants