eio icon indicating copy to clipboard operation
eio copied to clipboard

Relax return types of Eio.Process.Pipe

Open Arogbonlo opened this issue 1 year ago • 10 comments

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:

  • Relaxed Type Definitions: I’ve updated the return types for pipe so they can accept any subtype of Flow.source_ty and Flow.sink_ty. This will give us more flexibility when working with processes that require different types of flows.

  • Polymorphic Variants: I used polymorphic variants ([< .. ]) to allow for more flexible types, ensuring that the code still works with the broader variants expected in other parts of the system.

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.

Arogbonlo avatar Oct 18 '24 11:10 Arogbonlo

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

Thank you.

Arogbonlo avatar Oct 18 '24 11:10 Arogbonlo

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

Thank you.

Arogbonlo avatar Oct 18 '24 17:10 Arogbonlo

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 avatar Oct 20 '24 19:10 Arogbonlo

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 avatar Oct 22 '24 13:10 Arogbonlo

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?

Arogbonlo avatar Oct 22 '24 14:10 Arogbonlo

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 ?

patricoferris avatar Oct 22 '24 14:10 patricoferris

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 avatar Oct 22 '24 15:10 Arogbonlo

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 avatar Oct 22 '24 23:10 Arogbonlo

Hello @talex5 @patricoferris ,

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

Arogbonlo avatar Oct 24 '24 06:10 Arogbonlo

hey @talex5 @patricoferris.

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

Arogbonlo avatar Oct 24 '24 11:10 Arogbonlo

hey @talex5 @patricoferris.

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

Arogbonlo avatar Oct 27 '24 10:10 Arogbonlo