[wpilib] Make GameSpecificMessage optional #7445
Conversation
| HAL_MatchInfo info; | ||
| HAL_GetMatchInfo(&info); | ||
| return std::string(reinterpret_cast<char*>(info.gameSpecificMessage), | ||
| info.gameSpecificMessageSize); | ||
| if (info.gameSpecificMessage) { |
There was a problem hiding this comment.
Is it actually null or is it zero-length?
There was a problem hiding this comment.
My guess is 0 length based off the compile error
There was a problem hiding this comment.
No, this isn't the sort of thing you should be guessing at, it can be empirically (or otherwise) determined.
Especially When making a PR for a public project that is used by others, we probably shouldn't be guessing at things that are testable and/or knowable. There are certainly things that one has to guess at because it isn't possible to know it, or it involves a judgement call of some kind. This isn't one of those sorts of things (after all, how can you be sure that you fixed the bug that the PR is trying to fix without testing it?), and recognizing when that's the case and doing the extra work when needed will make you a better developer in the long run.
I don't typically make these sorts of critiques, but I do recognize that you've made a lot of high quality contributions to WPILib and seem to be very capable, and so I hope this thought will positively impact you and anyone else who reads this conversation.
There was a problem hiding this comment.
I do plan to test this and as such probably should've marked this as a draft. I more meant guess it's the correct approach to take before testing it (ie test this approach first) but that's a very valid criticism and I definitely should've made that more clear, my apologies!
Resolves wpilibsuite#7444 Signed-off-by: Jade Turner <[email protected]>
spacey-sooty
force-pushed
the
optional-game-specific-info
branch
from
a122170
to
2dfa985
Compare
|
This isn’t the correct fix. 0 length it not the same as not sent. This will require Hal changes. |
|
Since #7444 was closed (FIRST stated empty string will never be a valid value), can this be closed? |
|
Closing as OBE |
Resolves #7444