Add ProcessRunner trait to azure_core #2184
Conversation
arpad-m
requested review from
heaths,
RickWinter,
JeffreyRichter,
LarryOsterman and
ronniegeraghty
as code owners
github-actions
bot
added
Azure.Core
|
Thank you for your contribution @arpad-m! We will review the pull request and get back to you soon. |
arpad-m
changed the title
There was a problem hiding this comment.
Thank you. This is closer, but there are still a few things we'd like to see different. I can finish this up. Just make sure that maintainers can push to your branch and I'll get to it soon. I have some other higher priority work I need to attend to first but will leave this in my queue.
| @@ -0,0 +1,51 @@ | |||
| use async_trait::async_trait; | |||
There was a problem hiding this comment.
Missing copyright headers found in other files.
| tokio_fs = ["typespec_client_core/tokio_fs"] | ||
| tokio_process = ["dep:tokio", "tokio/process"] | ||
| tokio_sleep = ["typespec_client_core/tokio_sleep"] |
There was a problem hiding this comment.
Note to self: we're creating enough tokio-based features that maybe we should have a tokio feature that pulls them all in.
| #[cfg(not(feature = "tokio_process"))] | ||
| mod thread; | ||
|
|
||
| #[cfg(not(feature = "tokio_process"))] | ||
| pub use thread::ThreadRunner; | ||
|
|
||
| #[cfg(feature = "tokio_process")] | ||
| pub use tokio::TokioRunner; | ||
|
|
||
| #[cfg(feature = "tokio_process")] | ||
| mod tokio { |
There was a problem hiding this comment.
It'd be better to just isolate these into separate modules and conditionally pull them in if enabled.
| #[cfg(not(feature = "tokio_process"))] | ||
| mod thread; |
There was a problem hiding this comment.
You should never use mutually-exclusive features as called out in rust docs. The resolver does a union.
There was a problem hiding this comment.
There is no feature that's mutually exclusive with tokio_process: either it's present, or it's not. If there would be multiple features, from multiple backends, then there would be a problem indeed. But this isn't the case here.
There was a problem hiding this comment.
I'm fine with there being no default implementation if you disable our default. In that case, it's up to the callers to provide one and is the current plan with an internal team focused on Azure services.
| [target.'cfg(not(target_arch = "wasm32"))'.dependencies] | ||
| async-process.workspace = true | ||
|
|
There was a problem hiding this comment.
This should be removed from the workspace if it's the only one referencing it left.
I made this PR from a company owned fork of the repository which is a bit locked down. The "allow maintainers to do changes" checkbox doesn't even appear when making new pull requests from it. But feel free to cherry-pick changes from my branch to a new PR that you file. |
arpad-m
force-pushed
the
arpad/async-abstraction-2
branch
from
a47a129 to
dbc1036
Compare
|
To note: I haven't forgotten about this and will be working on it soon. I have a couple more things I want to get done first. |
Adds a
ProcessRunnertrait toazure_core, similar toHttpClient, and uses it in the one place inazure_identitywhere we usedasync-processbefore. We provide two implementations:successor of #1654, #2175
fixes #1652