Repository navigation
Error in return type for most 'autorange' properties in nidcpower library #2030
Description
Description of issue
In the code documentation, most of the "autorange"-related parameter are described as bool, which is wrong.
The correct type is int.
Steps to reproduce issue
``` python
import pytest
import nidcpower
@pytest.fixture
def smu() -> nidcpower.Session:
return nidcpower.Session(
resource_name="PXI1Slot2",
channels=None,
reset=True,
options={'range_check': True,
'simulate': True,
'driver_setup': {'Model': '4163', 'BoardType': 'PXIe'}},
independent_channels=True)
def test_wrong_type(smu):
assert isinstance(smu.autorange, bool)
def test_correct_type(smu):
assert isinstance(smu.autorange, int)
```
- Run pytest on the above script.
- The first test fails because an AssertiorError is raised.
- The second test passes.
Activity
marcoskirsch commentedon Nov 28, 2023
Hello,
Looking at the
nidcpower.Sessioncode, I see that the type of propertyautorangeisbool, which matches documentation.You may be thinking
boolis wrong because the NI-DCPower C API defines the attribute asViInt32. 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!
Arkh42 commentedon Nov 28, 2023
Hello,
I do not think that
boolis 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._SessionBasein thesessionmodule, proves that the documentation is wrong, as Python sees the returned type as anint.marcoskirsch commentedon Nov 28, 2023
I see, I misread the code.
However
bool(as documented) is right.Returning
intis wrong, we don't use magicintvalues in the Python API. A property like this would beboolif possible, its own enum type if not.Arkh42 commentedon Nov 28, 2023
It happens.
Thanks for considering it again. :)
marcoskirsch commentedon Nov 28, 2023
Were there other properties in the API that seem wrong?
Arkh42 commentedon Nov 30, 2023
Here is a list of the properties that are documented as
boolbut are in factintin Python:autorangecurrent_limit_autorangecurrent_level_autorangevoltage_limit_autorangevoltage_level_autorangeEDIT: I tested all of them in a similar fashion as the above test script provided when I opened the issue.
ni-jfitzger commentedon Dec 6, 2023
Based on what @marcoskirsch has told me, we should have done something like this with an enum for each of these properties:
Key points:
converted_value_to_enum_function_nameenum_to_converted_value_function_nameconverts_to_valuefor 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.