Skip to content

Conversation

@bparks13
Copy link
Member

@bparks13 bparks13 commented Oct 29, 2025

This PR is based on the changes made in #535, and goes a step further to ensure that the gain correction is non-null, throwing an explicit exception if it is ever null so that the error message is useful.

However, during testing it was noted that #535 actually made it difficult to test this PR. If the issue-505 branch is removed, then these modifications correctly handle the case initially described in the issue where the gain correction value could be null. Nominally though, there should never be a case in the Data* nodes where the serial number exists but the gain correction value is null, since that condition is explicitly tested for in the Configure* nodes.

Fixes #506

@bparks13 bparks13 added this to the 0.7.0 milestone Oct 29, 2025
@bparks13 bparks13 requested review from aacuevas and jonnew October 29, 2025 22:17
@bparks13 bparks13 self-assigned this Oct 29, 2025
@bparks13 bparks13 linked an issue Oct 29, 2025 that may be closed by this pull request
@bparks13 bparks13 removed the request for review from jonnew October 30, 2025 19:48
@aacuevas
Copy link
Collaborator

aacuevas commented Nov 3, 2025

With the new changes, this the code should never reach here with a null value.
So at this point, this null-check becomes more of a tool for us to locate a bug in the code leading to it.
Without it, we'd just have the normal null reference exception. With it, we have whatever we want to write it.

Personally, I believe checking for nullable values is generally a good practice and makes it obvious on the code that it should never we null. But maybe I'd replace the message for something that implies an error instead of an incorrect user action.

@bparks13
Copy link
Member Author

bparks13 commented Nov 3, 2025

@aacuevas That's a good point, I was writing the error message from the perspective before adding the other checks, which would have indicated incorrect user actions. I've updated the error message to indicate this is simply an error for it to be null, and not anything the user did.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NP headstage probe missing error message

3 participants