Impl: support extension of two urls comparison. #854
Conversation
AlexStocks
requested review from
beiwei30 and
Patrick0308
and removed request for
beiwei30
common/url_tool_extension.go
Outdated
| } | ||
|
|
||
| // Config default defaultURLComparator. | ||
| func init() { |
There was a problem hiding this comment.
Should be moved after the package declaration
There was a problem hiding this comment.
remove the file. Just use func instead of Interface.
common/url_tool_extension_test.go
Outdated
| package common | ||
|
|
||
| import ( | ||
| "github.com/apache/dubbo-go/common/constant" |
common/url_tool_extension.go
Outdated
|
|
||
| // default comparison implements | ||
| func (defaultURLComparator) CompareURLEqual(l *URL, r *URL, excludeParam ...string) bool { | ||
| return IsEquals(*l, *r, excludeParam...) |
There was a problem hiding this comment.
Do we need to check l, r whether is nil? And if one of them is nil, I think we should return false.
But if both of them are nil, I am not sure whether we should return true.
There was a problem hiding this comment.
@flycash 我们需要一个更系统性的 URL 改进方案，必须加锁确保安全，然后至少添加一个 Copy 函数，目前的实现很容易被人误用。
There was a problem hiding this comment.
我的理解是，现在URL只有我们这种开发维护人员才会直接使用到。误用是指哪方面？
There was a problem hiding this comment.
Add some check in IsEquals func.
common/url_tool_extension.go
Outdated
| type URLComparator interface { | ||
| CompareURLEqual(*URL, *URL, ...string) bool | ||
| } |
There was a problem hiding this comment.
May be a simple function is better?
like this var CompareURLEqualFunc(*URL, *URL, ...string) bool;
Codecov Report
@@ Coverage Diff @@
## develop #854 +/- ##
========================================
Coverage 60.20% 60.20%
========================================
Files 260 260
Lines 12886 12894 +8
========================================
+ Hits 7758 7763 +5
+ Misses 4170 4167 -3
- Partials 958 964 +6
Continue to review full report at Codecov.
|
|
LGTM |
common/url.go
Outdated
| return IsEquals(l, r, excludeParam...) | ||
| } | ||
|
|
||
| func init() { |
Impl: support extension of two urls comparison.
support extension of two urls comparison.
Because the URL.IsEquals will consume some performance, you can define your own implements.