-
Notifications
You must be signed in to change notification settings - Fork 586
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
Add convenience func to reverse callbacks and refactor other methods #7142
Add convenience func to reverse callbacks and refactor other methods #7142
Conversation
negotiatedVersions = slices.Clone(negotiatedVersions) | ||
slices.Reverse(negotiatedVersions) |
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.
this is the one thing I don't like about this PR, the negotiatedVersions are provided in reverse order.
I think we can either do this, require the caller to provide the calling fn to reverse them (easy to forget), or just reverse directly without copying, which will likely not be a problem but this version is just being extra careful.
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.
Hmm, yeah. Not beautiful, but its all part of this legacy module construction so I'm fine with it
Quality Gate passed for 'ibc-go'Issues Measures |
negotiatedVersions := make([]string, len(im.cbs)) | ||
|
||
for i := len(im.cbs) - 1; i >= 0; i-- { | ||
var negotiatedVersions []string |
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.
[nit] we could be slightly more efficient by doing:
negotiatedVersions := make([]string,0, len(im.cbs))
which would pre-allocate the needed capacity (but not the length).
If we know there's a 1:1 relationship between im.cbs
and negotiatedVersions
(so that the length is exactly the same) we could even do
negotiatedVersions := make([]string,len(im.cbs)
for i, cb := range im.reverseCallbacks() {
...
negotiatedversions[i] =
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.
yeah no harm!
// reverseCallbacks returns a copy of the callbacks in reverse order. | ||
// the majority of handlers are called in reverse order, so this can be used | ||
// in those cases to prevent needing to iterate backwards over the callbacks. | ||
func (im *LegacyIBCModule) reverseCallbacks() []ClassicIBCModule { |
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.
[nit] since this doesn't actually reverse the callbacks, but returns the "reversed", what about calling it reversedCallbacks
?
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.
sgtm, I can't think of any better naming tbh
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.
lgtm!
negotiatedVersions = slices.Clone(negotiatedVersions) | ||
slices.Reverse(negotiatedVersions) |
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.
Hmm, yeah. Not beautiful, but its all part of this legacy module construction so I'm fine with it
// reverseCallbacks returns a copy of the callbacks in reverse order. | ||
// the majority of handlers are called in reverse order, so this can be used | ||
// in those cases to prevent needing to iterate backwards over the callbacks. | ||
func (im *LegacyIBCModule) reverseCallbacks() []ClassicIBCModule { |
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.
sgtm, I can't think of any better naming tbh
Description
closes: #7135
This PR adds a reversal function and refactors the callbacks to reduce the amount of index manipulation used through the implementations.
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/
).godoc
comments.Files changed
in the GitHub PR explorer.SonarCloud Report
in the comment section below once CI passes.