Skip to content
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

Join the Udp_flow.partial_flow and Udp_flow.t types #3

Open
rand00 opened this issue Mar 8, 2023 · 13 comments
Open

Join the Udp_flow.partial_flow and Udp_flow.t types #3

rand00 opened this issue Mar 8, 2023 · 13 comments
Assignees
Labels
good first issue Good for newcomers

Comments

@rand00
Copy link
Owner

rand00 commented Mar 8, 2023

These two types became almost the same type over time, where the only difference is that the type t has the extra field feeder.

I think the best solution will be to make the feeder field into an 'a option, and then cancel only if it's Some feeder.

@rand00 rand00 added the good first issue Good for newcomers label Mar 8, 2023
@haanhvu
Copy link

haanhvu commented Mar 9, 2023

Hi, I have finished first steps in mirage/mirage#1402. I'll work on this now.

@rand00
Copy link
Owner Author

rand00 commented Mar 10, 2023

Hi @haanhvu - cool!

@haanhvu
Copy link

haanhvu commented Mar 10, 2023

@rand00 Hi, for now I have 2 questions:

  1. What is the name of the new type? Since we merging two types, I'm thinking of the merged name Udp_flow.partial_flow_t?
  2. I have compiled conntest successfully on main branch. However I use VSCode and wonder if there's a VSCode extension that checks compilation errors in real time? I actually installed OCaml Platform. But I didn't see it check compilation errors (don't know if there're further steps to do to use this extension for compilation checking).

Update: I installed ocaml-lsp-server and syntax errors are checked now. So there's only question1 left^^

@haanhvu
Copy link

haanhvu commented Mar 10, 2023

@rand00 Also, now I have another question: To make the feeder field into an 'a option as you said, do we just need to code feeder : 'a option? I did this and it gives me The type variable 'a is unbound in this type declaration error. Sorry very new to Ocaml syntax!

@rand00
Copy link
Owner Author

rand00 commented Mar 13, 2023

What is the name of the new type?

As the partial_flow type was named because it was a subset of t, the resulting name should be t. In OCaml t is the convention for the name of the primary type of a module - in this case Udp_flow.t.

Good that you got the realtime typechecking to work (:

@haanhvu
Copy link

haanhvu commented Mar 14, 2023

What is the name of the new type?

As the partial_flow type was named because it was a subset of t, the resulting name should be t. In OCaml t is the convention for the name of the primary type of a module - in this case Udp_flow.t.

Good that you got the realtime typechecking to work (:

@rand00 Thanks! Could you answer the other question I asked here too?

@rand00 Also, now I have another question: To make the feeder field into an 'a option as you said, do we just need to code feeder : 'a option? I did this and it gives me The type variable 'a is unbound in this type declaration error. Sorry very new to Ocaml syntax!

@rand00
Copy link
Owner Author

rand00 commented Mar 14, 2023

@rand00 Also, now I have another question: To make the feeder field into an 'a option as you said, do we just need to code feeder : 'a option? I did this and it gives me The type variable 'a is unbound in this type declaration error.

The feeder already has a static type, so there is no need to make it a type-parameter on t. (In case you wanted to, you would write type 'a t = ...). In this way the type of the feeder field would become unit Lwt.t option

@haanhvu
Copy link

haanhvu commented Mar 14, 2023

The feeder already has a static type, so there is no need to make it a type-parameter on t. (In case you wanted to, you would write type 'a t = ...). In this way the type of the feeder field would become unit Lwt.t option

@rand00 Thanks. I changed feeder field type as you said. So in the current code:

let flow = {
              ...
              feeder;
            }

What should feeder be changed to to compile with the new type?

Update: I just studied briefly about Ocaml option and it seems like I have to change feeder to feeder = Some feeder? Don't know if it's true or not but the type error's gone away after the change^^

@rand00
Copy link
Owner Author

rand00 commented Mar 15, 2023

I think when the questions become more code-specific, the discussion should move into a specific pull-request. If you have any questions around this just ask. Just prefix the PR with "WIP: ..." when it's not done yet

Update: I just studied briefly about Ocaml option and it seems like I have to change feeder to feeder = Some feeder? Don't know if it's true or not but the type error's gone away after the change^^

Indeed (: The OCaml compiler is very helpful in stating what is wrong - so try to fix all the type-errors from what it complains about - then it's often the case that the resulting code is correct

@rand00
Copy link
Owner Author

rand00 commented Mar 23, 2023

Hey @haanhvu - are you having progress?

@haanhvu
Copy link

haanhvu commented Mar 23, 2023

@rand00 I have been having exams since last week. I'll continue this this weekend.

@Khadija411
Copy link

Hi @rand00 If this issue is not solved I would like to work on it.

@haanhvu
Copy link

haanhvu commented Apr 3, 2023

Hi @rand00 If this issue is not solved I would like to work on it.

Yes it's not resolved. Feel free to take it up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants