`Frame.pdu_*` properties should be considered for a tuple by chlc3d · Pull Request #245 · ni/nixnet-python

chlc3d · 2018-03-28T22:22:49Z

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).

What does this Pull Request accomplish?

This PR implements issue #53 by removing the pdu_* properties with a single pdu properties. The old interface required parallel lists, this implementation uses a single list of namedtuples.

Why should this Pull Request be merged?

#53

What testing has been done?

I tested this with my CAN device by creating a PDU that met the requirements of PDUs Required?. I verified I could configure and take measurements without any warnings. If I didn't conform to PDUs required, I got either a warning or error depending on the case.

chlc3d · 2018-03-28T22:23:03Z

Since CAN/LIN currently only supports a subset of PDU features (one PDU per frame, a start bit of 0, and an update bit of -1), an alternative implementation would be to only have a pdu_ref function which takes a PDU item. We could assume the start bit of 0 and update bit of -1. However, this would be a departure from the XNET C interface, and we'd need to switch back to something like namedtuples if CAN/LIN ever supported more types of PDUs.

coveralls · 2018-03-28T22:27:33Z

Coverage increased (+9.5%) to 68.172% when pulling f52ce6c on trstamper:pdu into 4004cd3 on ni:master.

coveralls · 2018-03-28T22:27:34Z

Coverage increased (+3.2%) to 67.766% when pulling 043b296 on trstamper:pdu into 29f23de on ni:master.

epage

Don't forget to clean up your commits

epage · 2018-03-28T22:27:31Z

nixnet/database/_frame.py

-        _props.set_frame_pdu_update_bits(self._handle, value)
+    def pdus(self):
+        handles = [_pdu.Pdu(handle)
+                   for handle in _props.get_frame_pdu_refs(self._handle)]


While acceptable by flake8, I personally discourage this style because it leads to unnecessarily deep nesting, and simple refactors (like renaming handles) cause other lines to change (updating indentation)

Alternatively you can do

handles = [ _pdu.Pdu(handle) for handle in _props.get_frame_pdu_refs(self._handle)]

Sure. I was sticking with what autopep8 did, I don't have any horse in this fight.

I need to get an auto formatter setup. I hear good things about yapf. A new one recently came out but I can't find it.

epage · 2018-03-28T22:29:21Z

nixnet/database/_frame.py

+                           _props.get_frame_pdu_update_bits(self._handle)))
+        for pdu in [types.PduFrameReference(ref, start_bit, update_bit)
+                    for (ref, start_bit, update_bit) in pdu_tuples]:
+            yield pdu


Why create a list, then iterate over it, yielding the results? That feels overly complicated.

Instead you could just

return ( types.PduFrameReference(ref, start_bit, update_bit) for (ref, start_bit, update_bit) in pdu_tuples )

or

for (ref, start_bit, update_bit) in pdu_tuples]: yield types.PduFrameReference(ref, start_bit, update_bit)

Yeah, the last one looks cleaner to me too.

epage · 2018-03-28T22:30:45Z

nixnet/database/_frame.py

-    @pdu_update_bits.setter
-    def pdu_update_bits(self, value):
-        _props.set_frame_pdu_update_bits(self._handle, value)
+    def pdus(self):


Isn't Damon creating documentation for these? Are you two's commits going to step on each others toes?

Yep. I'll coordinate with damon on how to order these.

@trstamper #237 has been pulled, so whenever you're ready, you'll want to pull down master and merge it with your changes. There will be conflicts to resolve.

epage · 2018-03-28T22:31:11Z

nixnet/database/_frame.py

        """:any:`nixnet.database._dbc_attributes.DbcAttributeCollection`: Access the frame's DBC attributes."""
        if self._dbc_attributes is None:
-            self._dbc_attributes = _dbc_attributes.DbcAttributeCollection(self._handle)
+            self._dbc_attributes = _dbc_attributes.DbcAttributeCollection(


Why is this commit including an unrelated change?

I figured I would fix the pep8 violation (over the max line length) while I was in the file. I just looked at tox.ini and saw it's set to 120 characters max, so I'll change this back.

Our official guideline is to aim for below 80 characters but only enforce below 120.

... we still need to get that guideline published publicly.

d-bohls · 2018-04-06T17:14:22Z

nixnet/database/_frame.py


 from nixnet.database import _collection
 from nixnet.database import _dbc_attributes
+from nixnet.database import _pdu


This import will need to be moved down into the methods that use PDUs, else we will get circular imports when combined with #237.

I fixed this circular important but ran into another one trying to import nixnet.database._pdu in types.py because the PduProperties class has a Pdu object as one of its values and I wanted to reference it in the documentation. Why are we importing stuff for sphinx in every file instead of adding our whole library to the path in sphinx's conf.py?

Why are we importing stuff for sphinx in every file instead of adding our whole library to the path in sphinx's conf.py?

I don't follow. What imports are you talking about?

Damon and I talked about this offline and I found out I could avoid importing a class for sphinx to see it by not using the fully qualified name.

d-bohls · 2018-04-06T17:27:43Z

nixnet/database/_frame.py

+            yield pdu
+
+    @pdus.setter
+    def pdus(self, pdus):


#237 changes all ref methods from object_ref(s) to object(s) and returns the object instead of a ref. Thus, #237 has changed pdu_refs to pdus. Given that pattern applies to all the other database objects, calling your tuple pdus implies the method should return an iterable of PDU objects. Should we perhaps find another name then? Something like pdu_properties or pdu_info?

Also, you should return a PDU object instead of a handle as well. Just do something like Pdu(p.ref._handle)

#237 adds annotations and docstrings for all methods. Moving forward, these will be required for all methods, so you should add these things for your changes as well.

d-bohls · 2018-04-06T17:31:17Z

nixnet/types.py


+PduFrameReference_ = collections.namedtuple(
+    'PDU_',
+    ['ref', 'start_bit', 'update_bit'])


Per my previous comment, this could probably use some renaming since we are moving away from references to objects. My suggestions:

PduFrameReference_ -> PduProperties_
PDU_ -> PduProperties_
ref -> pdu

d-bohls

Looks good. Just a few requests.

d-bohls · 2018-04-19T16:03:47Z

nixnet/database/_frame.py

-        # type: () -> typing.Iterable[typing.Any]
+    def pdu_properties(self):
+        # type: () -> typing.Iterable[types.PduProperties]
        """list of :any:`Pdu`: Get or set a list that maps existing PDUs to a frame.


list of :any:`PduProperties`

d-bohls · 2018-04-19T16:05:31Z

nixnet/database/_frame.py

        """:any:`DbcAttributeCollection`: Access the frame's DBC attributes."""
        if self._dbc_attributes is None:
-            self._dbc_attributes = _dbc_attributes.DbcAttributeCollection(self._handle)
+            self._dbc_attributes = _dbc_attributes.DbcAttributeCollection(


No need to make this change.

Yeah whoops, must have missed that this got redone in the merge

d-bohls · 2018-04-19T16:09:59Z

nixnet/database/_frame.py

-            the sequence of values for the other two properties.
-        *   :any:`Frame.pdu_start_bits`: Defines the start bit of the PDU inside the frame.
-        *   :any:`Frame.pdu_update_bits`: Defines the update bit for the PDU inside the frame.
+        *   :any:`nixnet.types.PduProperties.ref`: Defines the sequence of values for the other two properties.


For consistency with other db docs

:any:`PduProperties.pdu` :any:`PduProperties.start_bit` :any:`PduProperties.update_bit`

Rename ref to pdu, per my comment later in the review.

d-bohls · 2018-04-19T16:12:22Z

nixnet/types.py



+PduProperties_ = collections.namedtuple(
+    'PDU_',


Seems like the typename of the named tuple should be something like PDU_PROPERTIES_ instead of PDU_. Not sure if this naming really matters.

We use handle/ref to talk about an integer that the driver uses for the object. We are dealing with an actual Pdu object here; not a ref. Change to:

['pdu', 'start_bit', 'update_bit'])

d-bohls · 2018-04-19T16:24:37Z

nixnet/database/_frame.py

        A mapped PDU is transmitted inside the frame payload when the frame is transmitted.
        You can map one or more PDUs to a frame and one PDU to multiple frames.

        Mapping PDUs to a frame requires setting three frame properties.


This isn't true anymore. Maybe something like:

Mapping PDUs to a frame requires modifying this list of :any:`PduProperties`, each of which requires the following three properties:

Yeahthat makes more sense. I was referring to the underlying driver calls here but that's not necessary at this spot.

d-bohls · 2018-04-19T16:32:31Z

nixnet/database/_frame.py

-    def pdu_update_bits(self, value):
-        # type: (typing.List[int]) -> None
-        _props.set_frame_pdu_update_bits(self._handle, value)
+        handles = [_pdu.Pdu(handle) for handle in _props.get_frame_pdu_refs(self._handle)]


pdus = [_pdu.Pdu(handle) for handle in ...

The handles have been converted to pdu objects.

chlc3d · 2018-04-20T15:16:06Z

Updated review to address comments.

jashnani

We should add unit tests for pdu_properties. This could be done part of #238 also. I'll leave it up to you, if you want to do it in this PR or coordinate with @d-bohls and do it in #238
I think you're introducing breaking changes. Its helpful if we call out API breakages in our commit messages. Also, it'll make it easier for us to find them (with a handy tool) when creating the release notes. Ideally, you include what someone needs to do to correct their code.

“””
Breaking Changes should start with the word BREAKING CHANGE: with a space or two newlines. The rest of the commit message is then used for this.
“””
From https://github.com/conventional-changelog/conventional-changelog/blob/master/packages/conventional-changelog-angular/convention.md

Example: 66e741f

BREAKING CHANGE: 'pdus', 'pdu_start_bits', and 'pdu_update_bits' properties have been removed from the Frame class. Use 'pdu_properties' instead.

chlc3d · 2018-04-20T18:18:38Z

Updated description to add BREKAING CHANGE tag

jashnani · 2018-04-27T05:26:46Z

For future reference, you can add Fixes #issue in the commit description and it will automatically close the issue when the commit is pulled into mainline.

epage reviewed Mar 28, 2018

View reviewed changes

jashnani assigned chlc3d Mar 29, 2018

jashnani self-requested a review March 29, 2018 14:03

d-bohls reviewed Apr 6, 2018

View reviewed changes

chlc3d force-pushed the pdu branch from 94710d8 to 4d20300 Compare April 18, 2018 18:27

d-bohls reviewed Apr 19, 2018

View reviewed changes

chlc3d force-pushed the pdu branch from 4d20300 to 2195634 Compare April 20, 2018 15:15

chlc3d force-pushed the pdu branch from 2195634 to 97c1f62 Compare April 20, 2018 15:56

jashnani reviewed Apr 20, 2018

View reviewed changes

fix(API): 'Frame.pdu_*' properties should be considered for a tuple

043b296

BREAKING CHANGE: 'pdus', 'pdu_start_bits', and 'pdu_update_bits' properties have been removed from the Frame class. Use 'pdu_properties' instead.

chlc3d force-pushed the pdu branch from 97c1f62 to 043b296 Compare April 20, 2018 18:18

jashnani approved these changes Apr 23, 2018

View reviewed changes

jashnani merged commit 024d1b8 into ni:master Apr 23, 2018

jashnani mentioned this pull request Apr 27, 2018

Frame.pdu_* properties should be considered for a tuple #53

Closed

Conversation

chlc3d commented Mar 28, 2018 • edited by jashnani Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this Pull Request accomplish?

Why should this Pull Request be merged?

What testing has been done?

Uh oh!

chlc3d commented Mar 28, 2018

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

epage left a comment

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!

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!

d-bohls left a comment

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!

chlc3d commented Apr 20, 2018

Uh oh!

jashnani left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chlc3d commented Apr 20, 2018

Uh oh!

jashnani commented Apr 27, 2018

chlc3d commented Mar 28, 2018 •

edited by jashnani

Loading

coveralls commented Mar 28, 2018 •

edited

Loading

coveralls commented Mar 28, 2018 •

edited

Loading

jashnani left a comment •

edited

Loading