Support for network ID above 255 by pierreluctg · Pull Request #1627

Support for network ID above 255 by pierreluctg · Pull Request #1627 · hardbyte/python-can

Choose a reason for hiding this comment

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

I would prefer this without partial. I see you want to avoid repetition, but it would be more readable if we just extract all parameters and use a single return Message(...) statement instead.

Choose a reason for hiding this comment

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

That would mean having a dictionary with the arguments, what's the difference with using partial, or what do you dislike about partial?

Choose a reason for hiding this comment

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

I don't mean a dictionary. I meant something simple like

timestamp = self._get_timestamp_for_msg(ics_msg)
arbitration_id = ics_msg.ArbIDOrHeader
 is_extended_id = bool(ics_msg.StatusBitField & ics.SPY_STATUS_XTD_FRAME)
is_remote_frame = bool(ics_msg.StatusBitField & ics.SPY_STATUS_REMOTE_FRAME)
is_error_frame = bool(ics_msg.StatusBitField2 & ics.SPY_STATUS2_ERROR_FRAME)
channel = ics_msg.NetworkID | (ics_msg.NetworkID2 << 8)
dlc = ics_msg.NumberBytesData
is_fd = is_fd
is_rx = not bool(ics_msg.StatusBitField & ics.SPY_STATUS_TX_MSG)

if is_fd:
    if ics_msg.ExtraDataPtrEnabled:
        data = ics_msg.ExtraDataPtr[: ics_msg.NumberBytesData]
    else:
        data = ics_msg.Data[: ics_msg.NumberBytesData]

    error_state_indicator = bool(ics_msg.StatusBitField3 & ics.SPY_STATUS3_CANFD_ESI)
    bitrate_switch = bool(ics_msg.StatusBitField3 & ics.SPY_STATUS3_CANFD_BRS)
else:
    data = ics_msg.Data[: ics_msg.NumberBytesData]   
    error_state_indicator = False
    bitrate_switch = False

return Message(
    timestamp=timestamp,
    arbitration_id=arbitration_id,
...
)

Choose a reason for hiding this comment

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

This means doing some assumption on some of the default keyword argument values.

In you example, you assume that the default value for error_state_indicator is False. What if the Message class changes and now the default is None.

This was one of the reason to go with partial.

Choose a reason for hiding this comment

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

Creating a partial instance in every recv() call might be inefficient and slow. But i didn't measure it of course.
You could argue that it's only too slow once somebody complains 😄

That said, i understand your reasoning. Feel free to merge.

Choose a reason for hiding this comment

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

Looking at the CPython partial implementation, I believe this overhead will be negligible.