Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a trio repl #2972
Add a trio repl #2972
Changes from 12 commits
85cee1b
8fa88ec
50ccf77
bc57699
9d2c724
f806db6
36752ea
339cff4
7c2b091
9d91484
c7336e9
2551d04
32b4b52
78d71bb
e7078d7
e83b96e
d6f4bd1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can some of this exception stuff be replaced with stuff from
outcome
? I don't see it providing much technical benefit but it would decrease some code duplication.Looking at outcome's code, it looks like they remove one layer off the exception's traceback where this doesn't. That might be a good thing to do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might not be understanding how outcomes works, but I don't see how it would simplify this. Wouldn't it just end up calling capture method and then immediately unwrapping?
The traceback printing is consistent with the standard python repl and the
python -m asyncio
so, I'm leaning towards keeping it as is.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well specifically I was thinking directly returning the Value/Error (and an
if
on whether to usecapture
oracapture
? Which would disallow leaving out theawait
when calling an async function but that's already trio style. I'll make this a separate comment when I get back to looking at this). If the trace backs are the same then the only benefit would be a few less lines, which probably isn't worth the work.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I came up with this. It's better in some ways, but at the same time bit strange.
I think it is awkward because there is a single step for most expressions, but any repl input that is an
await
needs the second step of evaluation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I finally tried poking at this a bit, ultimately what I was thinking about is:
probably doesn't work cause I haven't tested exactly that, but
inspect.iscoroutinefunction
seems to do the right thing (doesn't trigger fortrio.sleep(1)
orasync def x(): ...
, but triggers forawait trio.sleep(1)
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah,
iscoroutinefunction
is the bit I was missing. That is much better. Thanks for the nudge in that direction. I've got a few bits to double check but it looks like that works right.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally it might not be the best idea (since it can’t detect code that just returns a coroutine object directly). But in this case we know
return
isn’t valid.