ocaml-multicore / eio

Effects-based direct-style IO for multicore OCaml
Other
559 stars 72 forks source link

Relax return types of Eio.Process.Pipe #770

Closed Arogbonlo closed 3 weeks ago

Arogbonlo commented 1 month ago

This PR relaxes the return types of the pipe function in Eio.Process to make the code more flexible. The main goal was to allow the pipe function to handle a broader range of types for input and output flows, while still maintaining compatibility with the rest of the codebase.

Changes:

Testing: I ran dune runtest and confirmed that all tests are passing with these changes. The new types work fine, and the tests ensure everything is still functioning as expected.

Relaxing the types for pipe allows us to work with a wider variety of process setups and flows, without being too restrictive about the types we return. It also makes the code easier to extend in the future.

This addresses issue https://github.com/ocaml-multicore/eio/issues/750, improving flexibility when working with various APIs that use stricter type definitions for sources and sinks.

Arogbonlo commented 1 month ago

Hey @patricoferris. Please look at this and let me know if there are lapses.

Thank you.

Arogbonlo commented 1 month ago

Hey @patricoferris I've made modifications. I'd appreciate your looking into it.

Thank you.

Arogbonlo commented 1 month ago

hello @talex5 @patricoferris. Just made the corrections you highlighted. I'd appreciate if you take a look at it and let me know what you think...

thank you.

Arogbonlo commented 1 month ago

Hello @talex5 @patricoferris ,

I hope you're having a good day. Just a gentle reminder to have a check on this. Thank you for understanding.

Arogbonlo commented 1 month ago

There are still outstanding changes from @talex5's review

I had options of either casting in pipes or writing the cast in full as below:

let pipe (type tag) ~sw ((Resource.T (v, ops)) : [> tag mgr_ty] r) =
  let module X = (val (Resource.get ops Pi.Mgr)) in
  let r, w = X.pipe v ~sw in
  let r = (r : [Flow.source_ty | Resource.close_ty] r :> [< Flow.source_ty | Resource.close_ty] r) in
  let w = (w : [Flow.sink_ty   | Resource.close_ty] r :> [< Flow.sink_ty   | Resource.close_ty] r) in
  r, w

of which I chose to write in full. Or do you think otherwise about this my choice?

patricoferris commented 1 month ago

Yes the cast is now in val pipe of the Process interface. This means all the other places can go back to how they were I think. The problem is we would like to reduce the API changes being made (some of my original comments were wrong w.r.t to this).

Take a look at this commit 655bfed5d3047fa0dfcdbdf652384bb69c3061ae. These changes revert some of the changes already made, so it might be good to reset back to the start and just add the casts in the pipe leaving most of the code unchanged and force pushing here. Does that make sense ?

Arogbonlo commented 1 month ago

Yes the cast is now in val pipe of the Process interface. This means all the other places can go back to how they were I think. The problem is we would like to reduce the API changes being made (some of my original comments were wrong w.r.t to this).

Take a look at this commit 655bfed. These changes revert some of the changes already made, so it might be good to reset back to the start and just add the casts in the pipe leaving most of the code unchanged and force pushing here. Does that make sense ?

oh yeah, I get you. I'll make those corrections right away. Thank you.

Arogbonlo commented 1 month ago

hello @talex5 @patricoferris.

I just made the outstanding corrections you highlighted. I'd appreciate it if you take a look at it and let me know what you think...

Thank you.

Arogbonlo commented 4 weeks ago

Hello @talex5 @patricoferris ,

Just a gentle reminder to have a check on this. Thank you.

Arogbonlo commented 4 weeks ago

hey @talex5 @patricoferris.

I just made the modifications that were flagged. You can check them out. Thank you.

Arogbonlo commented 3 weeks ago

hey @talex5 @patricoferris.

I just made the changes. You can check them out. Thank you.