Skip to content
This repository has been archived by the owner on Nov 9, 2020. It is now read-only.

Extract haret-cli-client into a lib and bin crate #116

Merged
merged 1 commit into from
Jun 9, 2017

Conversation

erickt
Copy link
Contributor

@erickt erickt commented Jun 8, 2017

This starts pulling apart haret (#115), specifically cli client
binary into it's own module. The main reason to do this is for a
few reasons. First, it allows us to start framing out a higher
level library interface for Haret (haret-client). Second, it allows
us to shave off some dependencies if we only need a subset for a
particular application. haret-client will need a lot more attention
over time, since right now it just responds with string output.

Note that I've added a .gitignore to haret-client, as the
standard practice in the community is to only lock down
dependencies in the application crates, but leave it up to the
library consumers to decide what dependency versions they want
to use.

This starts pulling apart haret (vmware-archive#115), specifically cli client
binary into it's own module. The main reason to do this is for a
few reasons. First, it allows us to start framing out a higher
level library interface for Haret (haret-client). Second, it allows
us to shave off some dependencies if we only need a subset for a
particular application. haret-client will need a lot more attention
over time, since right now it just responds with string output.

Note that I've added a .gitignore to `haret-client`, as the
standard practice in the community is to only lock down
dependencies in the application crates, but leave it up to the
library consumers to decide what dependency versions they want
to use.
@andrewjstone
Copy link
Contributor

Thanks @erickt. I also have a clojure client that is not yet open that is a bit cleaner from a design perspective as it isn't meant for interactive human use. I find it much nicer to read that code ;) I'll give this a look and get it merged in.

@andrewjstone
Copy link
Contributor

Oh, I think there was some confusion here. The cli client code was absolutely not intended to be re-used as a library. It was just a quickly hacked up binary for interactive use. The docs say it's not intended to ever implement the complete API because of the difficulty of dealing with binary data at the CLI, although I suppose that could be done by reading and writing files.

The intention for writing clients was to write them in their own native language using native idioms and interacting via protobuf.

It does appear, however, that doing things this way means we don't have a very useful rust client yet. It appears that's what you were trying to do with this PR. @erickt do you think we should merge this in as in and simply "cleanup" the library, or build one out from scratch to better represent the API instead and then re-implement the CLI client?

@andrewjstone
Copy link
Contributor

Looking through the code again, it appears you did a good job of separating things out. Any rewrite probably would look similar and just change the types to something other than strings. I think this is fine to merge in, but once again would like your opinion after reading what I wrote above before I do so.

@erickt
Copy link
Contributor Author

erickt commented Jun 9, 2017

Hello again :) I'm happy to meet whatever standards you want to lay out haret. I tend to work a bit iteratively. My plan was that to pull out the client/server code, then iterate on HaretClient to be a bit more rust-y, such as getting rid of it returning strings and instead proper types. With that, then we could start extracting the server, and start experimenting with alternative messaging formats like raw protobuf, grpc, or capnproto.

@andrewjstone
Copy link
Contributor

This plan sounds great. I'm headed out for lunch now but will review more completely after that. From what I can see now though I expect to just merge away.

@andrewjstone
Copy link
Contributor

LGTM. Thanks @erickt

@andrewjstone andrewjstone merged commit e8f9a7f into vmware-archive:master Jun 9, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants