Closed buckd closed 3 years ago
Thank you for adding the unit test to the template!
Bleep bloop!
LabVIEW Diff Robot here with some diffs served up hot for your pull request.
Notice something funny? Help fix me on my GitHub repo.
We should check the boolean return value of AddCustomDevice
in Add Custom Device.vi
, as it's possible this call can fail if a custom device with the specified name already exists. Otherwise the changes look great.
Bleep bloop!
LabVIEW Diff Robot here with some diffs served up hot for your pull request.
Notice something funny? Help fix me on my GitHub repo.
We should check the boolean return value of
AddCustomDevice
inAdd Custom Device.vi
, as it's possible this call can fail if a custom device with the specified name already exists. Otherwise the changes look great.
@rtzoeller Get Next Unique Label
prevents us from trying to add something with the same name, so that's not a concern. Is there another reason that boolean would be false without returning an error on the error wire?
Or should we not be trying to uniquify the name provided to us? In that case, yes we should check that.
Or should we not be trying to uniquify the name provided to us? In that case, yes we should check that.
I'll leave the judgement to you. I think there are valid arguments for both approaches.
My primary concern with uniquifying the name is that someone could write code which adds the custom device, then later attempts to retrieve it by name. If they don't read the name of the node returned by Add Custom Device
, they could get a node they aren't expecting. I doubt this would be an issue in practice though, so we can ignore.
My primary concern with uniquifying the name is that someone could write code which adds the custom device, then later attempts to retrieve it by name. If they don't read the name of the node returned by
Add Custom Device
, they could get a node they aren't expecting. I doubt this would be an issue in practice though, so we can ignore.
I agree with the sentiment that it's not likely to be a problem in practice. Also, since this is how we do it for NI-SWITCH, Engine Sim, and Ballard ARINC 429, I'm inclined to leave it like this for consistency.
What does this Pull Request accomplish?
Creates scripting API entry points for adding a custom device, removing a custom device, finding a single custom device, and finding all custom devices.
Fixes #105
Why should this Pull Request be merged?
These entry points are necessities for starting from a good, known system definition.
What testing has been done?
Built locally and ran unit test.