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.

Sign up for GitHub

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

Jump to bottom

slice: 支持 Diff*, Union*, SymmetricDiff*, Intersection*, Index* 方法 #83

Merged
merged 1 commit into from
Sep 10, 2022
Merged

slice: 支持 Diff*, Union*, SymmetricDiff*, Intersection*, Index* 方法 #83

merged 1 commit into from
Sep 10, 2022

Conversation

flycash
Copy link
Contributor

@flycash flycash commented Sep 9, 2022

No description provided.

@flycash flycash mentioned this pull request Sep 9, 2022
Closed
@flycash
Copy link
Contributor Author

flycash commented Sep 9, 2022

看看我有没有漏掉你的 review，没有修改的

@flycash
Copy link
Contributor Author

flycash commented Sep 9, 2022

同时感谢 @penbox

@flycash flycash force-pushed the slice-helper branch 2 times, most recently from 4f370ac to 8ff85e1 Compare September 9, 2022 15:24
@flycash flycash mentioned this pull request Sep 9, 2022
Closed
@codecov
Copy link

codecov bot commented Sep 9, 2022 •
edited
Loading

Codecov Report

Merging #83 (f73003f) into dev (f5650a7) will increase coverage by 4.50%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev      #83      +/-   ##
==========================================
+ Coverage   91.40%   95.90%   +4.50%     
==========================================
  Files          20       21       +1     
  Lines         721      879     +158     
==========================================
+ Hits          659      843     +184     
+ Misses         54       28      -26     
  Partials        8        8
Impacted Files Coverage Δ
slice/contains.go 100.00% <100.00%> (+100.00%) ⬆️
slice/diff.go 100.00% <100.00%> (ø)
slice/index.go 100.00% <100.00%> (+100.00%) ⬆️
slice/intersect.go 100.00% <100.00%> (ø)
slice/map.go 100.00% <100.00%> (+100.00%) ⬆️
slice/symmetric_diff.go 100.00% <100.00%> (ø)
slice/union.go 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@flycash flycash mentioned this pull request Sep 9, 2022
Closed
@flycash
Copy link
Contributor Author

flycash commented Sep 9, 2022

同时感谢 @penbox

加到了合作者中

@flycash
Copy link
Contributor Author

flycash commented Sep 9, 2022

容量预估和非去重的版本，以后再实现

@flycash flycash linked an issue Sep 9, 2022 that may be closed by this pull request
slice： 切片辅助方法 #69
Closed
longyue0521
longyue0521 requested changes Sep 10, 2022
View reviewed changes
slice/contains.go Outdated Show resolved Hide resolved
slice/diff_test.go Show resolved Hide resolved
slice/index.go Outdated Show resolved Hide resolved
slice/intersect_test.go Show resolved Hide resolved
slice/intersect_test.go Outdated Show resolved Hide resolved
slice/symmetric_diff_test.go Outdated Show resolved Hide resolved
slice/symmetric_diff_test.go Outdated Show resolved Hide resolved
slice/union.go Outdated Show resolved Hide resolved
slice/union_test.go Show resolved Hide resolved
slice/union_test.go Show resolved Hide resolved
@longyue0521
Copy link
Collaborator

测试用例设计的不够全面,只考虑一般情况,边界情况未考虑. 后续补全、重构吧

@flycash
Copy link
Contributor Author

flycash commented Sep 10, 2022

改了，再看看 @longyue0521

@longyue0521
Copy link
Collaborator

longyue0521 commented Sep 10, 2022 •
edited
Loading

请仔细查看上面全部review,未折叠的表示问题还在或需要澄清 @flycash

@flycash
Copy link
Contributor Author

flycash commented Sep 10, 2022

应该都好了。nil 的那个，我改成了都返回空切片

@flycash
Copy link
Contributor Author

flycash commented Sep 10, 2022

后面你可以点 Resolve。一般就是谁开的对话，谁来 resolve

longyue0521
longyue0521 requested changes Sep 10, 2022
View reviewed changes
slice/map.go
func Map[Src any, Dst any](src []Src, m func(idx int, src Src) Dst) []Dst {
return []Dst{}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Map让人感觉是从src[]T 构造map, 改为Mapping或者Convert, ConvertTo怎么样?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个是 map reduce API 中的 map。或者我们可以考虑有一个专门的 mapreduce 包，这样这个方法就可以重命名为 ConvertTo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

但是专门的 mapreduce 包看起来不是特别好，因为在其它数据结构里面也可以引入类似的 API

Copy link
Contributor Author

Choose a reason for hiding this comment

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

依旧保留这个命名吧，我下一步就是要引入 reduce API。最开始我是计划在 Sum 的那个合并请求里面引入的

slice/map_test.go
"github.com/stretchr/testify/assert"
)

func TestMap(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

如果上方的Map要改,TestMap, ExampleMap也需要修改

slice/symmetric_diff.go Outdated
}
}

return ret
Copy link
Collaborator

Choose a reason for hiding this comment

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

ret需要去重, src = []int{1, 1, 2, 2, 3, 3}, dst = []int{2,2, 3, 3, 4, 4}; 添加测试用例并修复

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好了

@longyue0521
Copy link
Collaborator

longyue0521 commented Sep 10, 2022 •
edited
Loading

force push对review不太友好,会丢失之前review的对话,导致只能人工对比,效率很低. 多个commit可以在合并到dev的时候压缩成一个commit并修改comment. 不使用force push时,可以在review时使用如下选项提高效率. force push后点下面那个选项什么也看不到只能重新review.

review

@longyue0521
Copy link
Collaborator

longyue0521 commented Sep 10, 2022 •
edited
Loading

后面你可以点 Resolve。一般就是谁开的对话，谁来 resolve

这个Resolve是不是指review对话框右上角"..."里的hide->resolve

另外,当review过多时github展示时会hidden几个,隐藏的这几个我并没点resolve所以问题一直存在.我把原review关了,又开的新review. 下次看到“Load more”要点开看看,里面可能有问题未必解决

@flycash
Copy link
Contributor Author

flycash commented Sep 10, 2022

image

@flycash
Copy link
Contributor Author

flycash commented Sep 10, 2022

你说的每次 force push 确实是一个问题。

从排查问题的角度来说，一个合并请求只有一个 commit id 是稍微有点帮助的。但是我发现很多人不会 rebase，以及你提到的 review 的问题，所以我觉得后面可以不做这种要求。

@flycash flycash merged commit d8f9921 into ecodeclub:dev Sep 10, 2022
@flycash flycash mentioned this pull request Sep 10, 2022
Closed
@longyue0521
Copy link
Collaborator

longyue0521 commented Sep 10, 2022 •
edited
Loading

@flycash 不知道什么原因,我看到的对话框和你看到的不一样,我这没有那个按钮

所以当我再次review时我能做的只是点击右侧三个点->hide->reslove comment以表示这个问题解决了.

reslove_coversion

@longyue0521
Copy link
Collaborator

longyue0521 commented Sep 10, 2022 •
edited
Loading

@flycash

从排查问题的角度来说，一个合并请求只有一个 commit id 是稍微有点帮助的。但是我发现很多人不会 rebase，以及你提到的 review 的问题，所以我觉得后面可以不做这种要求。

想要达到一个PR合并到dev时一个commit, 你在合并PR的时候可以试试Squash and merge选项,还可看看另一篇文档,将PR中的多个commit压缩成一个并自定义最终的message,应该可以达到你想要的效果. 上面的链接中有图示,更直观一些.

这个方法如果可行,PR中就可以不限制commit数了,review也能方便点

@longyue0521
Copy link
Collaborator

@flycash

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/commenting-on-a-pull-request#resolving-conversations

大意是如果PR是我开的即我是被review的人,我可以看到resolve conversation按钮, 还有就是对ekit有写权限的人能够看到.

看这个意思resolve conversation是给被review的人用的,修改完了点一下,相当于在对话下回复了一个”已修改“.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@longyue0521 longyue0521 longyue0521 requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

slice： 切片辅助方法
3 participants
@flycash @longyue0521 @jackj-ohn1