-
Notifications
You must be signed in to change notification settings - Fork 43
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 NetworkManager.onLoadingFinished #1084
Conversation
1a939f5
to
01f0eef
Compare
This is a prior to fixing the bug #1072.
01f0eef
to
93ca042
Compare
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 🚀
Just some minor comments about the code comments 🙂
Co-authored-by: Ankur <ankur.agarwal@grafana.com>
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.
@@ -347,37 +347,52 @@ func (m *NetworkManager) onLoadingFailed(event *network.EventLoadingFailed) { | |||
} | |||
|
|||
func (m *NetworkManager) onLoadingFinished(event *network.EventLoadingFinished) { | |||
req := m.requestFromID(event.RequestID) | |||
rid := event.RequestID |
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.
s/rid/reqID ?
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.
Sorry, too long :) It feels more idiomatic to use rid
. Thanks for the suggestion, though.
What?
Refactors
NetworkManager.onLoadingFinished
to fix linter warnings.Why?
This is before fixing bug #1072. Otherwise, We'd have to do the refactoring in #1078.
Checklist
Related PR(s)/Issue(s)
Related: #1078