Catch and expound upon the py38 proactor add_reader() not implemented exception#88
Catch and expound upon the py38 proactor add_reader() not implemented exception#88altendky wants to merge 14 commits intopytest-dev:mainfrom
Conversation
|
Hopefully there's some useful code here though I suspect it's not all that sensible. A bit tired etc but wanted to get something in place as a reference to consider. |
altendky
changed the title
| six.raise_from( | ||
| value=AddReaderNotImplementedError.build(), | ||
| from_value=e, | ||
| ) |
There was a problem hiding this comment.
Needs some else: raise otherwise you suppress the exception
There was a problem hiding this comment.
Like I said, late and sleepy... :| Will do.
| "Failed to install asyncio reactor. The proactor was" | ||
| " used and is lacking the needed `.add_reader()` method" | ||
| " as of Python 3.8 on Windows." | ||
| " https://twistedmatrix.com/trac/ticket/9766" |
There was a problem hiding this comment.
This error message could use some love. The proactor doesn't implement add_reader, period, not just on 3.8.
More to the point, why the single-case exception? I think it would be better to just raise NotImplementedError with that message in the except block.
There was a problem hiding this comment.
The proactor might implement .add_reader() in the future. But sure, we know for <= 3.8.1.
I like specific exceptions and not incidentally catching those I don't intend by libraries providing their own. Look at the hoops to figure out if it is the NotImplementedError from .add_reader(). But I did consider inheriting from NotImplementedError since it is a raise from situation.
| _, _, traceback_object = sys.exc_info() | ||
| stack_summary = traceback.extract_tb(traceback_object) | ||
| source_function_name = stack_summary[-1].name | ||
| event_loop = asyncio.get_event_loop() |
There was a problem hiding this comment.
I'm -1 on this. I realize you're trying to scope it to this specific case to avoid a lying error message, but IMO this relies way too heavily on implementation details.
I think preserving the traceback is enough for context, and then just change the error to say what we think happened... Phrased like "If you're on windows and python 3.8+, you might need to..." perhaps.
There was a problem hiding this comment.
Perhaps my head was still in the 'identify the issue and resolve it' mode where you want to be sure. Given this is just going to refine the error message which can be worded as a guess it is more acceptable to... guess.
Maybe instead of more granular NotImplementedErrord raisers should include the function object raising it for identification.
Plus I suppose back to 'this is really twisted's problem to address' justifying less effort.
cdunklau
left a comment
cdunklau
left a comment
There was a problem hiding this comment.
I think a simpler detection routine would be better.
|
I'll take a pass responding to the rest later. |
|
Well, I responded already... but a pass of code change responses. |
|
Wait, I'm holding the ball... |
altendky
changed the title
2 participants
#80
https://twistedmatrix.com/trac/ticket/9766
https://twistedmatrix.com/trac/ticket/9809