refactor(API): have config_status return status code and text. by d-bohls · Pull Request #233 · ni/nixnet-python

d-bohls · 2018-03-08T05:38:50Z

BREAKING CHANGE:

Before, we just returned the status code.
However, the user previously had no way to get the text for the status code.
Now, return the status code and text in a tuple.

This contribution adheres to CONTRIBUTING.md.
New tests have been created for any new features or regression tests for bugfixes.
tox successfully runs, including unit tests and style checks (see CONTRIBUTING.md).

coveralls · 2018-03-08T05:42:50Z

Coverage increased (+0.2%) to 67.746% when pulling e0b74ad on d-bohls:dbConfigStatusString into e60323c on ni:master.

epage · 2018-03-08T15:24:04Z

FYI "read state" is another case where status codes are returned. When I added support for it, I ended up making it a check_fault function.

See

Should these different cases be consistent, one way or the other? Or is there a reason for us to take distinct approaches?

d-bohls · 2018-03-08T16:44:52Z

Regarding Ed's comment on "read state":

I talked to numerous people about this and got varying opinions. George H. was of was of the opinion that we should just return Fault from "read state" and not throw an exception because non-zero Fault indicates a network issue; not an XNET/driver issue. I think this needs a bit more research to determine what makes the most sense for a python user. I'm thinking I'll probably open a separate issue on what to do with Fault.

d-bohls · 2018-03-08T17:16:13Z

Clearly, what we currently have for config_status is insufficient. We return a status code and no way for the user to call nx_status_to_string to get text for the code.

Here are a few options:

Have config_status return a tuple containing the status code and text (this solution I've posted here). For a status code of zero, the string could be empty or none.
Handle the status just like other status return values: emit a warning or raise an exception for non-zero status.
Maybe we could make status a class that exposes code and text.

Other options? Thoughts?

jashnani · 2018-03-26T19:44:27Z

I could see us going either way on this. config_status returning None can be somewhat unclear as to whether or not the status is good. For consistency with check_fault, we could rename this property to check_config or something similar. So we would raise an exception on a bad error code and do nothing otherwise. It'll make the intent behind returning None more clear.

chlc3d · 2018-03-26T21:10:35Z

Throwing an exception with the status code and error message seems better to me than returning a tuple

d-bohls · 2018-04-17T18:34:15Z

I've looked into this a bit more. If we used tuples, the code might look something like this:

        signal_status = signal.config_status
        if signal_status[0] != 0:
            print(signal_status[0])  # print the error code
            print(signal_status[1])  # print the error text

Keep in mind these could be named tuples too.

If we throw an exception instead, the code looks like this:

        try:
            signal.check_config_status()
        except XnetError as e:
            print(e.error_code)  # print the error code
            print(str(e))  # print the error text

I ran tests on a database with an invalid signal, and the output for both of these looks like this:

3220582728
NI-XNET:  (Hex 0xBFF63148) Frame ConfigStatus: A signal within the frame exceeds the frame boundaries (Payload Length).

This exposes a few issues with the code, which are as follows:

The C API treats status as an i32. Positive codes are a warning and negative codes are an error. The python API treats status as a u32. It's an error if the MSB is set, and a warning otherwise. If the user is dealing with the code in hex, this is no big deal. However, equivalent error codes as int will not match between the C API and the python API. This doesn't seem ideal to me. Do we care?
The issue with unsigned status affects us in other ways too. In _cconsts.py line 8 we define NX_STATUS_ERROR = (0x80000000) which comes from the C API and is used as a bit mask for the MSB on i32 status codes. When we do this in python 2.7, NX_ERROR_BASE get promoted from an int to a long because 3220582400 (the unsigned value of 0x80000000) does not fit in 4 bytes for a signed python int. If we set NX_STATUS_ERROR to -2147483648 instead (the i32 value of 0x80000000), this promotion to long doesn't happen. Python 3.6 did away with long so NX_STATUS_ERROR stays an int. So far, the only effect I know of due to this issue is that I ran into difficulty trying to add typing checks on status codes, since codes are a long in 2.7 and an int in 3.6, and 3.6 doesn't know about longs.
Question: how should warning codes be handled when calling check_config_status? I talked to @sjasonsmith and it may be possible that config status on database objects could return a warning status code. If we send the status code through _errors.check_for_error, error codes will raise an exception but warnings will not. Does this match user expectation? Should warning codes also raise an exception in this particular case?

d-bohls · 2018-04-17T19:01:00Z

The last commit was just to show how I would be implementing the check_config_status method. Disregard the broken CI. Once we settle on the design, I will add a commit to this PR with the correct docstrings and this will fix the CI issue.

jashnani · 2018-04-17T19:34:46Z

This exposes a few issues with the code, which are as follows:

For # 1 and 2, would it make sense to open a separate issue and track it outside of this PR? I don't think it should be gating the config_status stuff.

For # 3, "how should warning codes be handled when calling check_config_status?" I was wondering the same thing. Maybe we should come up with an API guideline for how to expose check_status or check_whatever type of functionality to the Python API user. I have some ideas here but I can discuss those later after this release.

For consistency with check_fault and for other reasons mentioned in the review comments, for now I think we should just raise an error on a bad error code and return None otherwise.

BREAKING CHANGE: Before, we just returned the status code. However, the user previously had no way to get the text for the status code. Now, return the status code and text in a tuple.

or issues an XnetWarning if status is non-zero.

d-bohls · 2018-04-18T06:15:51Z

I filed issue #250 for the previously mentioned status code issues, and modified check_status_configuration so that it raises exception or warns. This is ready for review now.

epage · 2018-04-18T14:09:43Z

nixnet/_utils.py

-            _errors.check_for_error(_cconsts.NX_ERR_INVALID_PROPERTY_VALUE)
+            error_code = _cconsts.NX_ERR_INVALID_PROPERTY_VALUE
+            error_text = _errors.status_to_string(error_code)
+            raise errors.XnetError(error_text, error_code)


Why are you no longer calling check_for_error?

I see there is a dedicated commit for this but nothing in the message gives an explanation for it.

Why do you want check_for_error to be called? If I know I want to raise an error, why would I call a method that conditionally raises or warns instead of raising the error directly? Originally, I thought this was the "abuse" you were mentioning in the comments, though given your questioning here I suspect you meant the abuse was that you just picked the best fitting error code. That wasn't clear to me.

Side note: in another PR I have, calling check_for_error instead of raising directly confuses mypy into complaining that there is a path without a return value.

Why do you want check_for_error to be called? If I know I want to raise an error, why would I call a method that conditionally raises or warns instead of raising the error directly?

My primary concern is that changes are explained in the commit message.

My (slight) preference for check_for_error is that it is a one-stop shop for getting the appropriate behavior in the correct way. I kept XnetError's __init__ "low level" for the sake of being easy to create whatever type of XnetError we need and assumed everyone would create them via check_for_error.

Originally, I thought this was the "abuse" you were mentioning in the comments, though given your questioning here I suspect you meant the abuse was that you just picked the best fitting error code. That wasn't clear to me.

Yes, it is referring to the error code

Side note: in another PR I have, calling check_for_error instead of raising directly confuses mypy into complaining that there is a path without a return value.

One way of resolving this is we could add a dependency on mypy_extensions and use its NoReturn type annotation.

On the other hand, check_for_error is not unconditionally a NoReturn due to the warning case. Hmmm that is one thing in favor of not using check_for_error.

What would you think about adding a method in _errors.py

def raise_error(error_code):

that creates XnetError and raises it. Then, those two methods in _errors.py would be our one-stop shop and my hope is that mypy would be smart enough to figure out that a code path involving raise_error unconditionally throws.

I added a new method raise_xnet_error to use when we unconditionally want an error, and which is called by check_for_error. This also resolve the mypy return issue.

epage · 2018-04-18T14:09:49Z

nixnet/_utils.py

-        _errors.check_for_error(_cconsts.NX_ERR_INVALID_PROPERTY_VALUE)
+        error_code = _cconsts.NX_ERR_INVALID_PROPERTY_VALUE
+        error_text = _errors.status_to_string(error_code)
+        raise errors.XnetError(error_text, error_code)


epage · 2018-04-18T14:10:23Z

nixnet/database/_frame.py

-            _errors.check_for_error(_cconsts.NX_ERR_SIGNAL_NOT_FOUND)
+            error_code = _cconsts.NX_ERR_SIGNAL_NOT_FOUND
+            error_text = _errors.status_to_string(error_code)
+            raise errors.XnetError(error_text, error_code)


epage · 2018-04-18T14:10:36Z

nixnet/database/_lin_sched_entry.py

-            _errors.check_for_error(_cconsts.NX_ERR_DATABASE_OBJECT_NOT_FOUND)
+            error_code = _cconsts.NX_ERR_DATABASE_OBJECT_NOT_FOUND
+            error_text = _errors.status_to_string(error_code)
+            raise errors.XnetError(error_text, error_code)


epage · 2018-04-18T14:10:44Z

nixnet/database/_pdu.py

-            _errors.check_for_error(_cconsts.NX_ERR_SIGNAL_NOT_FOUND)
+            error_code = _cconsts.NX_ERR_SIGNAL_NOT_FOUND
+            error_text = _errors.status_to_string(error_code)
+            raise errors.XnetError(error_text, error_code)


epage · 2018-04-18T14:10:52Z

nixnet/database/_signal.py

-            _errors.check_for_error(_cconsts.NX_ERR_FRAME_NOT_FOUND)
+            error_code = _cconsts.NX_ERR_FRAME_NOT_FOUND
+            error_text = _errors.status_to_string(error_code)
+            raise errors.XnetError(error_text, error_code)


We have been using check_for_error, which conditionally raises an error, in cases where we want to unconditionally raise an error. This confuses mypy into thinking there is a possible code path that doesn't produce a return value. I'm adding raise_xnet_error to unconditionally raise an error, which also resolve the mypy issue.

jashnani · 2018-04-18T17:07:27Z

nixnet/_utils.py


 def parse_lin_comm_bitfield(first, second):
-    # (int) -> types.CanComm
+    # type: (int, int) -> types.LinComm


Good catch!

d-bohls added the breaking-change label Mar 8, 2018

d-bohls self-assigned this Mar 8, 2018

d-bohls requested a review from jashnani March 8, 2018 05:38

d-bohls force-pushed the dbConfigStatusString branch from 061e66b to 8a6a277 Compare March 13, 2018 02:58

chlc3d mentioned this pull request Apr 6, 2018

docs(DB): Finish all the database annotations/docs. #237

Merged

3 tasks

d-bohls force-pushed the dbConfigStatusString branch from 8a6a277 to 2978216 Compare April 17, 2018 18:39

d-bohls force-pushed the dbConfigStatusString branch 2 times, most recently from c388a40 to ac39a1c Compare April 17, 2018 21:38

d-bohls mentioned this pull request Apr 17, 2018

Sign and type issues with XNET status codes #250

Open

d-bohls added 2 commits April 17, 2018 23:49

refactor(API): have config_status return status code and text.

aee21b3

BREAKING CHANGE: Before, we just returned the status code. However, the user previously had no way to get the text for the status code. Now, return the status code and text in a tuple.

check_configuration_status throws an XnetError

fb9db27

or issues an XnetWarning if status is non-zero.

d-bohls force-pushed the dbConfigStatusString branch 2 times, most recently from ad533b5 to 2175e60 Compare April 18, 2018 05:46

epage suggested changes Apr 18, 2018

View reviewed changes

d-bohls added 2 commits April 18, 2018 11:49

fix(API): fix incorrect type annotation.

e0b74ad

d-bohls force-pushed the dbConfigStatusString branch from 2175e60 to e0b74ad Compare April 18, 2018 16:55

jashnani approved these changes Apr 18, 2018

View reviewed changes

nixnet/_utils.py

def parse_lin_comm_bitfield(first, second):

# (int) -> types.CanComm

# type: (int, int) -> types.LinComm

Copy link

jashnani Apr 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

jashnani merged commit 29f23de into ni:master Apr 18, 2018

d-bohls deleted the dbConfigStatusString branch April 19, 2018 03:26

Conversation

d-bohls commented Mar 8, 2018

Uh oh!

coveralls commented Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

epage commented Mar 8, 2018

Uh oh!

d-bohls commented Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

d-bohls commented Mar 8, 2018

Uh oh!

jashnani commented Mar 26, 2018

Uh oh!

chlc3d commented Mar 26, 2018

Uh oh!

d-bohls commented Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

d-bohls commented Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jashnani commented Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

d-bohls commented Apr 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

epage Apr 18, 2018 • edited by d-bohls Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coveralls commented Mar 8, 2018 •

edited

Loading

d-bohls commented Mar 8, 2018 •

edited

Loading

d-bohls commented Apr 17, 2018 •

edited

Loading

d-bohls commented Apr 17, 2018 •

edited

Loading

jashnani commented Apr 17, 2018 •

edited

Loading

epage Apr 18, 2018 •

edited by d-bohls

Loading