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

refactor: remove func ErrWhenBuildListIdx #443

Merged

Conversation

TremblingV5
Copy link
Contributor

This pr come from #403

Remove func: ErrWhenBuildListIdx

Founded that ErrWhenBuildListIdx only has been called in buildListIdx, which only return an error. So I use fmt.Errorf to wrap the returned error where buildListIdx is called.

Additionally, Go 1.18 no longer has errors.Wrap, so I use fmt.Errorf to finish this pr.

This's my first pr in this repo, correct me if something wrong. : )

@codecov
Copy link

codecov bot commented Aug 20, 2023

Codecov Report

Merging #443 (4ee18a2) into master (0eae1bc) will decrease coverage by 0.51%.
Report is 5 commits behind head on master.
The diff coverage is 47.22%.

@@            Coverage Diff             @@
##           master     #443      +/-   ##
==========================================
- Coverage   66.97%   66.46%   -0.51%     
==========================================
  Files          34       34              
  Lines        5910     5892      -18     
==========================================
- Hits         3958     3916      -42     
- Misses       1606     1620      +14     
- Partials      346      356      +10     
Files Changed Coverage Δ
db.go 63.68% <47.22%> (+1.56%) ⬆️

... and 2 files with indirect coverage changes

@bigboss2063
Copy link
Member

bigboss2063 commented Aug 20, 2023

感觉可以在 buildListIdx 里面返回 error 的时候,就用 fmt.Errorf 来指明是在进行什么操作的时候出的错,可以参考 buildSetIdx 方法:
图片
如果有空的话可以也帮忙将 buildSortedSetIdx 里面的错误返回也改成这样吗?😁

@bigboss2063 bigboss2063 self-requested a review August 20, 2023 11:02
db.go Outdated
@@ -856,7 +856,7 @@ func (db *DB) buildOtherIdxes(r *Record) error {
switch r.H.Meta.Ds {
case DataStructureList:
if err := db.buildListIdx(r); err != nil {
return err
return fmt.Errorf("when build listIdx err: %s", err)
Copy link
Member

Choose a reason for hiding this comment

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

The string can be defined into a const string variable. That will make the code more elegant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The string can be defined into a const string variable. That will make the code more elegant.

OK : )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The string can be defined into a const string variable. That will make the code more elegant.

I refactored code so that fmt.Errorf was only called once and I didn't use const to define this pattern string, because I think it would be more intuitive if only called once.

@elliotchenzichang
Copy link
Member

Thanks for this significant contribution, and that can be an excellent first issue.

@TremblingV5
Copy link
Contributor Author

buildSetIdx

感觉可以在 buildListIdx 里面返回 error 的时候,就用 fmt.Errorf 来指明是在进行什么操作的时候出的错,可以参考 buildSetIdx 方法: 图片 如果有空的话可以也帮忙将 buildSortedSetIdx 里面的错误返回也改成这样吗?😁

好的,我是看一个switch case里面有很多个err需要包装,这个函数也只是返回err,所以想着在外面写会显得精简一些

@TremblingV5
Copy link
Contributor Author

感觉可以在 buildListIdx 里面返回 error 的时候,就用 fmt.Errorf 来指明是在进行什么操作的时候出的错,可以参考 buildSetIdx 方法: 图片 如果有空的话可以也帮忙将 buildSortedSetIdx 里面的错误返回也改成这样吗?😁

关于buildListIdx的错误包装我有个疑问:在这个函数中,并不是每个返回的err都调用了ErrWhenBuildListIdx,是否有什么特殊的考虑?我是否要把所有返回的err都使用相同方式进行包装?

@bigboss2063
Copy link
Member

没有特殊的考虑,应该只是前后的开发者没有统一上🤣,可以将 err 都这么包装一下

@TremblingV5
Copy link
Contributor Author

没有特殊的考虑,应该只是前后的开发者没有统一上🤣,可以将 err 都这么包装一下

好的

另外,我觉得在每个case都使用fmt.Errorf去进行包装显得有些啰嗦 ,不知道您对如下这种写法有什么看法:

var err error
switch EXPR {
case A:
    err = doSomething1()
case B:
    err = doSomething2()
case C:
    err = doSomething3()
    if err != nil {
        break
    }
    err = doSomething4()
case D:
    err = doSomething5()
}

if err != nil {
    return fmt.Errorf("some wrap words %s", err)
}

return nil

golang日志库zap中存在类似的写法

@bigboss2063
Copy link
Member

没有特殊的考虑,应该只是前后的开发者没有统一上🤣,可以将 err 都这么包装一下

好的

另外,我觉得在每个case都使用fmt.Errorf去进行包装显得有些啰嗦 ,不知道您对如下这种写法有什么看法:

var err error
switch EXPR {
case A:
    err = doSomething1()
case B:
    err = doSomething2()
case C:
    err = doSomething3()
    if err != nil {
        break
    }
    err = doSomething4()
case D:
    err = doSomething5()
}

if err != nil {
    return fmt.Errorf("some wrap words %s", err)
}

return nil

golang日志库zap中存在类似的写法

确实这是一种更优雅的方式 👍

@TremblingV5 TremblingV5 force-pushed the remove-func-ErrWhenBuildListIdx branch 2 times, most recently from c98ee41 to 060fdd3 Compare August 22, 2023 13:43
@TremblingV5 TremblingV5 changed the title remove func ErrWhenBuildListIdx refactor: remove func ErrWhenBuildListIdx Aug 23, 2023
@TremblingV5
Copy link
Contributor Author

没有特殊的考虑,应该只是前后的开发者没有统一上🤣,可以将 err 都这么包装一下

好的
另外,我觉得在每个case都使用fmt.Errorf去进行包装显得有些啰嗦 ,不知道您对如下这种写法有什么看法:

var err error
switch EXPR {
case A:
    err = doSomething1()
case B:
    err = doSomething2()
case C:
    err = doSomething3()
    if err != nil {
        break
    }
    err = doSomething4()
case D:
    err = doSomething5()
}

if err != nil {
    return fmt.Errorf("some wrap words %s", err)
}

return nil

golang日志库zap中存在类似的写法

确实这是一种更优雅的方式 👍

已经按照这种方式更改了buildListIdxbuildSortedSetIdx相关的代码

@bigboss2063
Copy link
Member

我又增加了一些评论,麻烦看一下😁

db_test.go Show resolved Hide resolved
db.go Outdated Show resolved Hide resolved
db.go Show resolved Hide resolved
db.go Outdated Show resolved Hide resolved
@TremblingV5 TremblingV5 force-pushed the remove-func-ErrWhenBuildListIdx branch from 7185b09 to ec9bb6a Compare August 23, 2023 13:43
@TremblingV5
Copy link
Contributor Author

我又增加了一些评论,麻烦看一下😁

好嘞,已经按照意见进行了一些修改

@bigboss2063
Copy link
Member

都用这种风格的话,buildSetIdx也统一这样用 switch case呗hhh,麻烦了,应该是最后一次修改🤣

@TremblingV5
Copy link
Contributor Author

都用这种风格的话,buildSetIdx也统一这样用 switch case呗hhh,麻烦了,应该是最后一次修改🤣

好滴没问题

@TremblingV5
Copy link
Contributor Author

都用这种风格的话,buildSetIdx也统一这样用 switch case呗hhh,麻烦了,应该是最后一次修改🤣

注意到在db.go中存在其他几个函数,也是和buildXXIdx这样的函数的结构类似:对同一个变量进行多次if判断然后执行操作,是否要把这些函数也都重构成swich-case的结构?

@bigboss2063
Copy link
Member

都用这种风格的话,buildSetIdx也统一这样用 switch case呗hhh,麻烦了,应该是最后一次修改🤣

注意到在db.go中存在其他几个函数,也是和buildXXIdx这样的函数的结构类似:对同一个变量进行多次if判断然后执行操作,是否要把这些函数也都重构成swich-case的结构?

这次先把构建索引的处理逻辑处理吧,可以一步一步来

@TremblingV5
Copy link
Contributor Author

都用这种风格的话,buildSetIdx也统一这样用 switch case呗hhh,麻烦了,应该是最后一次修改🤣

注意到在db.go中存在其他几个函数,也是和buildXXIdx这样的函数的结构类似:对同一个变量进行多次if判断然后执行操作,是否要把这些函数也都重构成swich-case的结构?

这次先把构建索引的处理逻辑处理吧,可以一步一步来

好滴

@TremblingV5
Copy link
Contributor Author

buildSetIdx

已经将buildSetIdx修改为switch-case的代码格式,鉴于此函数只有2个分支,且已有代码中对error的包装并不一样,所以没有采用提前声明err,在最后进行统一包装的方式。

Copy link
Member

@bigboss2063 bigboss2063 left a comment

Choose a reason for hiding this comment

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

LGTM! 感谢你的杰出贡献😁

@bigboss2063 bigboss2063 merged commit d1808de into nutsdb:master Aug 24, 2023
4 of 6 checks passed
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.

None yet

3 participants