Move MakeJoin logic to GMSL #3081
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #3081 +/- ##
==========================================
- Coverage 67.33% 66.29% -1.05%
==========================================
Files 497 497
Lines 53161 53790 +629
==========================================
- Hits 35795 35659 -136
- Misses 13802 14587 +785
+ Partials 3564 3544 -20
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
| } else if err != nil { | ||
| util.GetLogger(httpReq.Context()).WithError(err).Error("eventutil.BuildEvent failed") | ||
| if response == nil { | ||
| util.GetLogger(httpReq.Context()).Error("gmsl.HandleMakeJoin returned invalid response") |
There was a problem hiding this comment.
I strongly dislike this: we shouldn't be returning nil responses AND nil errors. If I see this log line in the logs it gives me no clue what went wrong here. Please return an error instead with contextual information. Returning nil, nil makes sense only when A) the data being nil isn't an error and B) the nil indicates absence rather than as a result of an OR operation (error OR data).
There was a problem hiding this comment.
We don't ever actually return nil, nil, but since it returns a pointer to the result it technically could have been nil.
The result is a pointer since it returns a full ProtoEvent struct. I could change that to be behind a pointer, but that could also technically be nil too.
Which we could just not check, and just let the server panic assuming it was a programmer error that led to this case. I really don't like leaving in intentional panics across a library boundary. It's one thing to have them for internal programming errors, but relying on an external library api behaviour is dangerous.
There was a problem hiding this comment.
If it can never return nil, nil then why do we have this if statement? Can we remove it?
No description provided.