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

Functorize State.Context and make client vs server capability handling more explicit #441

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ulugbekna
Copy link
Contributor

  1. Functorize State.Context

Capability management in State.Context.t seems out of place because State is responsible for handling read-write monad and capability management is handled by Smart, which is State's user.

With the current change, higher-level context management such as handling capabilities is done at a higher layer, namely Smart module.

  1. Having a pair to denote client's and server's capabilities and unnamed arguments of type Capability.t list was error-prone imo. This PR makes it more explicit to discern more easily server and client caps. I am not sure whether "client" and "server" are good namings. Maybe "our_caps", "their_caps" could be better.

@dinosaure
Copy link
Member

I don't really understand why we need to functorize this part, I mean, the only usage of such functor is the application here:
https://github.com/mirage/ocaml-git/pull/441/files#diff-da0d4644201b7c89b6726ed7efd021a38cb025220a7a33159f19d6d376062dccR119-R121

The best should not be a simple extension/renaming of internal fields of the Context.t?

@ulugbekna
Copy link
Contributor Author

ulugbekna commented Jan 19, 2021

I find the old code slightly strange because:

  1. State defines a module Context and a functor Scheduler, which takes as params module types CONTEXT and VALUE.
    Smart instantiates State.Scheduler by passing State.Context to it, which makes Context param of the functor useless because State already could use Context without functorizing over it. Am I missing something?

  2. I also find it strange that State is responsible for defining capability management functionality that isn't used anywhere by State itself but is used by a user of State, Smart, which is located at a higher layer imo.


With the new code, State has a single purpose of carrying states of Encoder and Decoder AND some additional state that is required by the user of State, would it be capabilities and/or some other info which a user of State wants to carry in the "state" along with encoder and decoder.


I want to experiment with reusing State in the protocol v2, which has a different definition of Capability module, so if you're still not convinced about merging this and think that reusing State can be a more convincing reason for functorizing this code, let me know that you'd prefer to wait the end of my experiments.

@dinosaure
Copy link
Member

I want to experiment with reusing State in the protocol v2, which has a different definition of Capability module

In that case, I would like to prefer to merge Capv1 and Capv2 into one and single type. Then, fetch_v1/fetch_v2 can ignore others values (like function #Cap.V1.t as cap -> ... | #Cap.V2.t as cap -> ...).

I mostly would like to avoid any functors, specially on this part of ocaml-git and use as much as we can values.

* make client, server capabilities management more explicit
@ulugbekna
Copy link
Contributor Author

ulugbekna commented Jan 20, 2021

does this look better now? :-)

edit: we can keep it unmerged for now

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

Successfully merging this pull request may close these issues.

2 participants