Fixed Slcan fileno raising an exception on windows#760
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #760 +/- ##
===========================================
- Coverage 70.39% 69.79% -0.61%
===========================================
Files 70 70
Lines 6840 6488 -352
===========================================
- Hits 4815 4528 -287
+ Misses 2025 1960 -65 |
|
Better specify the error type. Naked excepts can hide bugs. |
d5122ac to
aef6921
Compare
|
@felixdivo @bessman I made the changes. What do you think? |
| try: | ||
| return self.serialPortOrig.fileno() | ||
| # Return an invalid file descriptor on Windows | ||
| except (NotImplementedError, AttributeError, io.UnsupportedOperation): |
There was a problem hiding this comment.
Can NotImplementedError or AttributeError actually be raised here? If not we shouldn't try to catch them. Based on the old code it looks like at least AttributeError could be raised. @christiansandberg, do you remember the reason for the hasattr check?
There was a problem hiding this comment.
That was probably a mistake from my end. I saw that pyserial did not implement the fileno() method but forgot to check if it was inherited instead.
There was a problem hiding this comment.
In that case, if the only exception that has actually been observed in the wild here is UnsupportedOperation, I suggest only trying to catch that. If it turns out other exceptions (that we want to catch) can be raised here, add them later.
aef6921 to
1b5d6f8
Compare
|
Changed to only catch io.UnsupportedOperation. |
|
I think an even better approach would be to implement the fileno() method in BusABC which would raise UnsupportedOperation. The slcan implementation would not need to handle the exception at all, but the Notifier would do that instead. Returning -1 instead of raising an exception was not very Pythonic I admit. |
|
@christiansandberg I like the general idea. |
9c3a70a to
d87cad7
Compare
|
Sorry for the inactivity. I thought we were leaning towards the solution offered by @christiansandberg . |
|
Ahh right you are. I've opened #877 if you'd like to check it out |
Slcan currently raises an exception when used with 'can.Notifier' on windows.
'can.Notifier.add_bus' calls 'bus.fileno' which calls 'serialPortOrig.fileno' which raises an exception on windows as it is not implemented.
The fix catches the exception raised by calling fileno instead of checking if it exists(as it can exist and still raise an error).