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

Better algebraic layout for Node type #4

Open
dzhus opened this issue Aug 24, 2015 · 3 comments
Open

Better algebraic layout for Node type #4

dzhus opened this issue Aug 24, 2015 · 3 comments

Comments

@dzhus
Copy link

dzhus commented Aug 24, 2015

Currently Node type allows semantically meaningless combinations of _nodeDir and _nodeValue (such as Just .. or Nothing for both). It also discourages users of the library from picking logical branches of code idiomatically (via pattern matching), forcing them to check for field values instead. Even if this is the only/simplest way available with JSON I don't see why it should be brought to Haskell land. I think that Node should be like this:

data Node = Node
    { _nodeKey           :: !Key
    , _nodeCreatedIndex  :: !Index
    , _nodeModifiedIndex :: !Index
    , _nodeContent       :: !NodeContent
    , _nodeTTL           :: !(Maybe TTL)
    , _nodeExpiration    :: !(Maybe UTCTime)
    } deriving (Show, Eq, Ord)


data NodeContent = Leaf !Value
                 | Children ![Node]

What do you think?

@wereHamster
Copy link
Owner

You are right, the current types are not ideal. I kept them as close to the JSON as possible because I didn't know the exact semantics of the etcd protocol. I was able to experiment with the etcd API and see how it behaves (its documentation wasn't great back then).

As for your proposed API, it should be possible to retrofit it into the current code without breaking the API, to not cause too much trouble to existing users of this library.

@ryantrinkle
Copy link

@wereHamster Would you be alright with a not-completely-backwards-compatible change, e.g. supplying deprecated getters, without supplying setters?

@wereHamster
Copy link
Owner

Actually, the API is small enough that any non-backwards compatible changes will be trivial to adapt to. So feel free to go ahead and rethink the whole Node type and related API. A nice readme, a proper ChangeLog, good haddock documentation, and a few well-documented examples will take us a long way ensuring users have a good experience when updating their projects.

In the long run, we'll need to move to API v3 which is based on gRPC anyway (because etcd v2 will no longer be shipped with Container Linux after June 2018), and I have no experience with accessing gRPC services from Haskell. I assume that Haskell gRPC tooling can automatically generate types from .proto files, and we'll want to expose those directly.

Another issue I identified with the current Haskell API is that each request (get, put etc) creates a new HttpManager which is rather expensive, it would be better to create the HttpManager once and store it in Client). This needs to be reworked, too, because of #5. But maybe it's not worth doing work on that because the Haskell gRPC library may take care of the low-level communication details for use.

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

No branches or pull requests

3 participants