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

Added Order() method support for gdb. #1529

Merged
merged 6 commits into from
Mar 9, 2022
Merged

Added Order() method support for gdb. #1529

merged 6 commits into from
Mar 9, 2022

Conversation

zxr615
Copy link
Contributor

@zxr615 zxr615 commented Dec 22, 2021

#1434

本来是想在 doQuoteString(s, charLeft, charRight string) 中做判断,但方法中的参数是 string 类型,而且 doQuoteString() 被引用的地方很多,如果改参数 sinterface{} 的话方法名也不太符合原意了,所以就直接在 Order() 方法里面修改了,多了个 for 循环,似乎不太优雅,大佬可以给给意见。


// Order sets the "ORDER BY" statement for the model.
//
// Eg:
// Order("id desc")
// Order("id", "desc")
Copy link
Member

Choose a reason for hiding this comment

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

这个注释不要删。

Copy link
Contributor Author

@zxr615 zxr615 Dec 22, 2021

Choose a reason for hiding this comment

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

好的,测试没通过debug 的时候发现个问题,这里的 if 判断似乎是无效的,因为下面的 model.orderBy 又被重新赋值了,所以如果链式多次使用 Order() 也是无效的,我看文档确实没有提到多次链式使用 Order() 是设计如此吗?

image

Copy link
Contributor

Choose a reason for hiding this comment

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

我也觉得25行应该改成 +=,但我PR #1520 的CI一直跑不完不知道为什么

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我也觉得25行应该改成 +=,但我PR #1520 的CI一直跑不完不知道为什么

改成 += 之后有些测试用例的结果和预期的是不一样的了,比如测试用例是:Order("status desc").Order("id asc"), 实际排序是只有id asc, 而改成 += 之后就变成了status desc id asc, 所以 asset() 结果的时候就不对了,你可以本地跑一遍测试用例就知道什么问题了

Copy link
Contributor

Choose a reason for hiding this comment

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

多谢,之前没看到github ci里的fail,还以为莫名其妙失败了

@gqcn
Copy link
Member

gqcn commented Dec 22, 2021

@zxr615 你的CI失败了

@gqcn
Copy link
Member

gqcn commented Jan 6, 2022

@zxr615 请继续修改撒,如果超过一个月没有活动会关闭哦

@codecov-commenter
Copy link

codecov-commenter commented Jan 7, 2022

Codecov Report

Merging #1529 (d308623) into master (aabc1f8) will decrease coverage by 0.41%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1529      +/-   ##
==========================================
- Coverage   70.18%   69.77%   -0.42%     
==========================================
  Files         434      438       +4     
  Lines       42020    42427     +407     
==========================================
+ Hits        29492    29603     +111     
- Misses      10652    10927     +275     
- Partials     1876     1897      +21     
Flag Coverage Δ
go-1.15 69.75% <100.00%> (-0.43%) ⬇️
go-1.16 69.72% <100.00%> (-0.38%) ⬇️
go-1.17 ?

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

Impacted Files Coverage Δ
database/gdb/gdb_model_order_group.go 55.00% <100.00%> (-0.89%) ⬇️
encoding/gjson/gjson_stdlib_json_util.go 70.58% <0.00%> (-29.42%) ⬇️
database/gdb/gdb_statement.go 28.57% <0.00%> (-22.95%) ⬇️
text/gstr/gstr_convert.go 82.82% <0.00%> (-17.18%) ⬇️
database/gredis/gredis_adapter_goredis_conn.go 65.67% <0.00%> (-11.12%) ⬇️
os/gspath/gspath_cache.go 84.21% <0.00%> (-10.53%) ⬇️
os/gcmd/gcmd_command_run.go 60.76% <0.00%> (-7.03%) ⬇️
database/gdb/gdb_result.go 32.14% <0.00%> (-6.99%) ⬇️
database/gredis/gredis_adapter_goredis.go 93.75% <0.00%> (-6.25%) ⬇️
... and 115 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aabc1f8...d308623. Read the comment docs.

@zxr615
Copy link
Contributor Author

zxr615 commented Jan 7, 2022

@zxr615 请继续修改撒,如果超过一个月没有活动会关闭哦

因为 Order() 支持 Order("id", "desc") 这样的多参数查询,所以有可能 Order("id", "desc", gdb.Raw("field(id, 3,1,2)")) 这样使用,如果要区分处理会比较复杂,所以使用一个循环检查如果有 gdb.Raw() 的话则直接使用 gdb.Raw() 条件,舍去其他参数;从我个人角度来说,应该没什么问题,理由:因为用了 gdb.Raw() 原始条件后应该是不会再拼接其他的参数,如果需要的话可以多次调用 Order() 来增加.

1.17 CI 失败了,不知道啥原因,麻烦大佬帮忙看看

@zxr615
Copy link
Contributor Author

zxr615 commented Jan 10, 2022

@zxr615 请继续修改撒,如果超过一个月没有活动会关闭哦

因为 Order() 支持 Order("id", "desc") 这样的多参数查询,所以有可能 Order("id", "desc", gdb.Raw("field(id, 3,1,2)")) 这样使用,如果要区分处理会比较复杂,所以使用一个循环检查如果有 gdb.Raw() 的话则直接使用 gdb.Raw() 条件,舍去其他参数;从我个人角度来说,应该没什么问题,理由:因为用了 gdb.Raw() 原始条件后应该是不会再拼接其他的参数,如果需要的话可以多次调用 Order() 来增加.

1.17 CI 失败了,不知道啥原因,麻烦大佬帮忙看看

@gqcn 这个CI提示端口的错误是什么原因呢?

@gqcn gqcn merged commit fa57634 into gogf:master Mar 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants