Open Arkh42 opened 9 months ago
Hello,
In the code documentation, most of the "autorange"-related parameter are described as bool, which is wrong. The correct type is int.
Looking at the nidcpower.Session
code, I see that the type of property autorange
is bool
, which matches documentation.
You may be thinking bool
is wrong because the NI-DCPower C API defines the attribute as ViInt32
. But while our Python and C APIs are very close matches, they do deviate in small specific ways enabled by the nature of the Python language. This is an example of that. Please check our Python API Design Guidelines for more detail.
Thanks for the feedback!
Hello,
I do not think that bool
is wrong because of the C API, but because Python tells me so. (See the pytest code example.)
The first link that you provided, which refers to nidcpower._SessionBase
in the session
module, proves that the documentation is wrong, as Python sees the returned type as an int
.
I see, I misread the code.
However bool
(as documented) is right.
Returning int
is wrong, we don't use magic int
values in the Python API. A property like this would be bool
if possible, its own enum type if not.
It happens. Thanks for considering it again. :)
most of the "autorange"-related parameter are described as bool, which is wrong.
Were there other properties in the API that seem wrong?
Here is a list of the properties that are documented as bool
but are in fact int
in Python:
autorange
current_limit_autorange
current_level_autorange
voltage_limit_autorange
voltage_level_autorange
EDIT: I tested all of them in a similar fashion as the above test script provided when I opened the issue.
Based on what @marcoskirsch has told me, we should have done something like this with an enum for each of these properties:
'IsolationState': {
'codegen_method': 'private',
'converted_value_to_enum_function_name': 'convert_to_isolation_state_enum',
'enum_to_converted_value_function_name': 'convert_from_isolation_state_enum',
'values': [
{
'converts_to_value': True,
'documentation': {
'description': 'The channel is disconnected from chassis ground.'
},
'name': 'NIDCPOWER_VAL_ISOLATED',
'value': 1128
},
{
'converts_to_value': False,
'documentation': {
'description': 'The channel is connected to chassis ground.'
},
'name': 'NIDCPOWER_VAL_NON_ISOLATED',
'value': 1129
}
]
},
Key points:
converted_value_to_enum_function_name
enum_to_converted_value_function_name
converts_to_value
for each valueBefore we make a change like this, we need to think through the implications and do some testing to confirm it's not a breaking change.
Description of issue
In the code documentation, most of the "autorange"-related parameter are described as
bool
, which is wrong. The correct type isint
.Steps to reproduce issue