Tyrrrz / CliWrap

Library for running command-line processes
MIT License
4.32k stars 264 forks source link

Implement `AsyncMethodBuilder` for `CommandTask<T>` to refactor some code #184

Closed Tyrrrz closed 1 year ago

Tyrrrz commented 1 year ago

Details

Currently, some parts of CliWrap's internal code use the following patterns to build upon CommandTask<T>:

https://github.com/Tyrrrz/CliWrap/blob/b956aaf4934ad735b3cad9d1d9d0422b11e1287c/CliWrap/Buffered/BufferedCommandExtensions.cs#L46-L76

https://github.com/Tyrrrz/CliWrap/blob/b956aaf4934ad735b3cad9d1d9d0422b11e1287c/CliWrap/EventStream/PullEventStreamCommandExtensions.cs#L65

This is because we can't use async/await in methods returning CommandTask<T> because C# compiler doesn't know how to construct instances of CommandTask<T>.

We should be able to resolve this by implementing a custom AsyncMethodBuilder for CommandTask<T>. Some resources on this topic:

The method builder should probably be internal to avoid introducing unnecessary liabilities. The end user is unlikely to define methods that return CommandTask<T> themselves. If this issue arises in the future, we can address it then.

Tyrrrz commented 1 year ago

Now that I think about it, this might not be possible to implement. That's because CommandTask<T> isn't really an async container in the same general sense as Task<T>.

More specifically, a method with async CommandTask<T> signature would only work if:

I doubt C# compiler can enforce these criteria for us. Perhaps, it's more practical to continue using the existing Select()/Bind() abstractions.

Tyrrrz commented 1 year ago

Decided it's not really worth it