-
Notifications
You must be signed in to change notification settings - Fork 847
Clean code and fix issues reported by staticcheck #223
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
base: master
Are you sure you want to change the base?
Conversation
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on Slack: #support-infosec. |
498236a to
e5b6f40
Compare
|
Any chance this PR could be reviewed? |
|
@sarroutbi Wondering the same about my own PR. The project seems pretty dead to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the time to clean these up! I just had a few nits, otherwise looks great.
|
|
||
| rt, err = mutable.Commit() | ||
| require.NoError(t, err) | ||
| rt, err = Load(cfg.Persister, rt.ID(), comparator) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we require.NoError here instead of underscoring?
btree/immutable/rt_test.go
Outdated
| require.NoError(t, err) | ||
| expected := toOrdered(items).toItems() | ||
| result, err := mutable.(*Tr).toList(itemsToValues(expected...)...) | ||
| result, _ := mutable.(*Tr).toList(itemsToValues(expected...)...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here:
Can we require.NoError here instead of underscoring?
btree/immutable/rt_test.go
Outdated
| mutable.AddItems(items[:25]...) | ||
| mutable.(*Tr).verify(mutable.(*Tr).Root, t) | ||
| rt, err := mutable.Commit() | ||
| rt, _ = mutable.Commit() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here as well:
Can we require.NoError here instead of underscoring?
e5b6f40 to
1da9ed0
Compare
Signed-off-by: Sergio Arroutbi <[email protected]>
1da9ed0 to
a69ce3b
Compare
|
Hello @matthinrichsen-wf . Can you please check, when possible, if this PR can be merged after your recommendations? Thanks !!! |
| } | ||
|
|
||
| outputBytes, err := Marshal(input) | ||
| outputBytes, _ := Marshal(input) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could assert on err to be complete.
| for _, key := range keys { | ||
| _, ok = hm[key] // or the compiler complains | ||
| } | ||
| for range keys { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure that the compiler is not going to drop this out?
| return true | ||
| return list.apply(low, high, func(n *node) bool { | ||
| return irt.apply(n.orderedNodes, interval, dimension+1, fn) | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might simplify this further by moving this out of the else which will also allow us to remove the return true below.
| return true | ||
| return list.apply(low, high, func(n *node) bool { | ||
| return ot.apply(n.orderedNodes, interval, dimension+1, fn) | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As in the immutable.go file, we can simplify this further.
|
Thank you for your contribution. I will see if @matthinrichsen-wf and I can get this merged in the coming weeks. |
No description provided.