Closed MJafarMashhadi closed 4 years ago
I think this is a good option to raise exceptions. however, I think there are a few other places where it should be catch properly, not only in ivy.py
. serial.py
, udp.py
and message.py
are also using messages_xml_map
. What do you think ?
Hi, thanks for you comment!
Here I handled the exception because the source of the error is outside the program, it happens when receives an unknown message and just ignores it which is robust and fine. There is a piece of code in pprz_transport
that does the same thing and I missed. I'll fix that in a few hours.
In other parts, the messages_xml_map usages were either safe (e.g. calling parse_messages
which doesn't raise any exceptions) or with valid parameters (e.g. constants in tests or values that are already checked).
The last case is in the PprzMessaage constructor which gets the inputs as parameters from the user. If the parameters are invalid it's the user's responsibility to handle the error, so I didn't add any error handling there.
There is a piece of code in
pprz_transport
that does the same thing and I missed. I'll fix that in a few hours.
I'm done now. Please review the new changes. Thanks!
As long as they are not sending telemetry messages (which I couldn't find any) they should be fine, but if there were any bug reports just mention me I'd be more than happy to fix 😅
--
send_raw_datalink
is used in:
datalink
messagessend
is used in:
AIRCRAFTS_REQ
which can be replaced with send_request
when #122 is mergedThat's all
thanks for the detailed review!
Thank you!
xref #116
Changes:
None
. It's just a best-practices in Python that gives the library user more options for handling errors.This changes the class's interface (
send
andsend_raw_datalink
methods' to be more specific) and could become backwards-incompatible. I cross-checked usages ofIvyMessagesInterface
with the main repository, including the submoduled projects. Most of them were only subscribing to certain messages and weren't using these methods. The ones which sent messages were not sendingtelemetry
messages that can in an special case raiseValueError
. To best of my knowledge, this is safe to merge, but it never hurts to have a second pair of more experienced eyes review it.