Cli client #157

binoychitale · 2021-01-25T15:47:38Z

Signed-off-by: binoychitale binoychitale@gmail.com

Fixes #141

ysimonson · 2021-01-25T22:45:48Z

client/src/main.rs

@@ -160,9 +163,11 @@ pub async fn main() -> Result<(), Box<dyn Error>> {
        .get_matches();

    let _address = matches.value_of("address").unwrap();


This should be renamed to address. Some of the variables in here were prefixed with an underscore temporarily to pass clippy, until they're used.

ysimonson · 2021-01-25T22:46:12Z

client/src/main.rs


    if let Some(_matches) = matches.subcommand_matches("ping") {
-        unimplemented!();
+        client.ping().await.map_err(|err| err.compat())?;
+        println!("PING OK");


Let's just print ok.

ysimonson · 2021-01-25T22:49:25Z

client/src/main.rs

    }
-
-    panic!("unknown command");


If match fails on any of the nested ifs, a panic won't occur with this change. A couple of options here:

Add the panic to each of the nested if arms.

Keep the old panic, but call into a function on any of the if arms. e.g.:

if let Some(_matches) = matches.subcommand_matches("ping") { return ping(client).await; } ...

As more code gets added, (2) will probably be cleaner.

Got it.
I also found a setting for the App/Subcommand which enforces that at least one of the subcommands must be used.

I think this will error out more gracefully than panicking manually.

Have pushed a few more changes.

If match fails on any of the nested ifs, a panic won't occur with this change.

By enforcing that "at least one" of the nested subcommands is used at runtime (using AppSettings::SubcommandRequiredElseHelp), we should not run into this condition

Oh cool, nice find!

ysimonson · 2021-01-26T19:12:56Z

client/src/main.rs

    } else if let Some(matches) = matches.subcommand_matches("set") {
        if let Some(_matches) = matches.subcommand_matches("vertex") {
-            unimplemented!();
+            let link_type = indradb::Type::new(_matches.value_of("type").unwrap()).map_err(|err| err.compat())?;


This would more appropriately be called vertex_type

ysimonson · 2021-01-26T19:13:06Z

client/src/main.rs


    if let Some(_matches) = matches.subcommand_matches("ping") {
-        unimplemented!();
+        client.ping().await.map_err(|err| err.compat())?;
+        println!("ok");
    } else if let Some(matches) = matches.subcommand_matches("set") {
        if let Some(_matches) = matches.subcommand_matches("vertex") {


The underscore prefix for this can be removed now

ysimonson · 2021-01-26T19:13:41Z

client/src/main.rs

+                .await.map_err(|err| err.compat())?
+                .create_vertex(&vertex)
+                .await.map_err(|err| err.compat())?;
+            if res == false {


This can be simplified to if !res {

ysimonson · 2021-01-26T19:14:49Z

client/src/main.rs

+                .create_vertex(&vertex)
+                .await.map_err(|err| err.compat())?;
+            if res == false {
+                panic!("Failed to create vertex with id: {} and type: {}", vertex.id, vertex.t.0);


This should return a new error rather than panicking

ysimonson · 2021-01-26T19:17:46Z

client/src/main.rs

+            if res == false {
+                panic!("Failed to create vertex with id: {} and type: {}", vertex.id, vertex.t.0);
+            }
+            println!("Created vertex with id: {} and type: {}", vertex.id, vertex.t.0);


Maybe just print the debug version of the vertex (i.e., println!("{:?}", vertex);), that way it should be easier to parse if this is used in a script

binoychitale · 2021-01-27T08:16:35Z

For setting vertex properties, we expect the vertex query as a JSON.
There are different types of vertex queries which would need to be de-serialized according to the specific type of vertex query.

Is there a sample JSON vertex query to use as a reference?
What would be the best way to de-serialize? using serde can be an option

ysimonson · 2021-01-27T22:28:56Z

In a separate PR, I'll make VertexQuery and EdgeQuery auto-derive serde::Deserialize. Once that's done, deserializing should be as simple as let query: VertexQuery = serde_json::from_str(arg_string)?. It would be the more pleasant JSON, but it'll work.

For deserializing to a straight-up JSON value, you should similarly be able to do something like let value: JsonValue = serde_json::from_str(arg_string)?.

ysimonson · 2021-01-28T00:47:36Z

Actually, this might not be as straight-forward as I'd hoped.

ysimonson · 2021-01-29T01:55:24Z

Instead of supporting full-blown vertex/edge queries, let's just support operations on individual vertices (by UUID) / edges (by edge key.) That'll make things a lot easier, and still deliver a good amount of value.

binoychitale · 2021-01-29T09:55:54Z

Since we're now only supporting vertex queries by Uuid and edges by EdgeKey, I suppose it no longer makes sense to have get vertex and get edge commands, since the only supported lookups would be by uuid and edgekey

ysimonson · 2021-01-29T12:57:39Z

There is still some marginal utility, in that you’d get back info on whether the vertex/edge exists, as well as it’s update time stamp. But yeah it’s pretty limited, and not needed for initial version of the client.

binoychitale · 2021-01-29T16:38:52Z

Got it. In that case ill just implement it, doesnt seem like too much effort

Signed-off-by: binoychitale <binoychitale@gmail.com>

ysimonson · 2021-01-30T16:51:36Z

client/src/main.rs

+    let edge_type_arg = Arg::with_name("type").help("the edge type").required(true);
+    let inbound_id_arg = Arg::with_name("inbound_id")
+        .help("the inbound vertex ID")
+        .required(true);


Do these args require either a .takes_value(true) or .index(true)? Maybe they don't, not sure.

ysimonson · 2021-01-30T16:57:28Z

lib/src/errors.rs

@@ -17,6 +17,8 @@ pub enum Error {
    Sled { inner: SledError },
    #[fail(display = "UUID already taken")]
    UuidTaken,
+    #[fail(display = "Invalid vertex")]
+    VertexInvalid,


This should be a separate error in the bin crate, since it's not produced in lib ever. Thankfully, because main returns Box<dyn Error>, different types of errors can be returned.

Correct me if wrong here but I think this actually would be useful in lib, because if create_edge returns a false from a non-CLI call, it would still be useful error to throw. (maybe pave the way to return an error instead of a boolean value from this method in the future)

At the library level, it's not considered an error to insert a redundant vertex. This has a couple of distinct advantages:

For datastores that provide all-or-none transaction semantics, inserting a vertex redundantly won't invalidate the entire transaction.

Similarly, for bulk inserts, this ensures that an attempt to create a vertex won't invalidate the entire bulk insert operation.

ysimonson · 2021-01-30T16:59:52Z

client/src/main.rs

+                Some(_) => indradb::EdgeDirection::Inbound,
+                None => indradb::EdgeDirection::Outbound,
+            };
+            let edge_type = match matches.value_of("type") {


You can use Option::map to simplify this a bit

I'm not sure how to cleanly bubble errors out of the closure without making the code equally verbose
For e.g if I write -

let edge_type = matches.value_of("type") .map(|edge_type| indradb::Type::new(edge_type).map_err(|err| err.compat()));

the error thrown by the new() method needs to be handled in the outer function

Ah right, yeah a match seems like the right move here

ysimonson · 2021-01-30T17:03:26Z

client/src/main.rs


-    if let Some(_matches) = matches.subcommand_matches("ping") {
-        unimplemented!();
+        println!("ok");


This is the only command that doesn't acquire a transaction, so to shorten the code, it might be good to restructure to something like this:

if matches.subcommand_matches("ping").is_some() { // ...ping return Ok(()); } let trans = client.transaction().await.map_err(|err| err.compat())?; // ...various subcommand matches here

ysimonson · 2021-01-30T17:04:24Z

client/src/main.rs

-        .help("the JSON edge query")
-        .required(true)
-        .index(1);
+    let edge_query_arg = [outbound_id_arg, edge_type_arg, inbound_id_arg];


This + the various build functions are a nice way to shorten the code 👍

Signed-off-by: binoychitale <binoychitale@gmail.com>

ysimonson · 2021-02-01T21:07:28Z

This looks almost ready to merge! Could you take a look at the warnings triggered by github actions in the diff? Once that's done, I'll take a final pass.

Signed-off-by: binoychitale <binoychitale@gmail.com>

binoychitale · 2021-02-02T06:50:32Z

I've resolved the clippy warnings, we're good to go :)

ysimonson · 2021-02-13T22:22:12Z

The CLI client just went out in the latest release (v2.1.0), and is available on linux/macos as a pre-built binary. Congrats and thanks again for this substantial contribution, @binoychitale!

binoychitale mentioned this pull request Jan 25, 2021

Add CLI client #141

Closed

ysimonson reviewed Jan 25, 2021

View reviewed changes

ysimonson reviewed Jan 26, 2021

View reviewed changes

ysimonson mentioned this pull request Jan 27, 2021

Make VertexQuery and EdgeQuery derive deserialize #159

Closed

binoychitale marked this pull request as ready for review January 29, 2021 17:42

binoychitale added 12 commits January 29, 2021 23:15

Add ping support via CLI

a7994c1

Signed-off-by: binoychitale <binoychitale@gmail.com>

Panic in case of unknown command

89a3677

Signed-off-by: binoychitale <binoychitale@gmail.com>

Support for create vertex via CLI

736c0bd

enforce at least one nested subcommand

3e1d42b

Signed-off-by: binoychitale <binoychitale@gmail.com>

Support create edge via CLI

d040629

Signed-off-by: binoychitale <binoychitale@gmail.com>

Support counting vertices and edges via CLI

19c4132

Signed-off-by: binoychitale <binoychitale@gmail.com>

Support setting vertex and edge properties via CLI

bc87423

Signed-off-by: binoychitale <binoychitale@gmail.com>

Support vertex and edge lookups via CLI

dfcf11f

Signed-off-by: binoychitale <binoychitale@gmail.com>

Support delete operations via CLI

16ba64f

Signed-off-by: binoychitale <binoychitale@gmail.com>

Add support for vertex and edge get using CLI

e67b02c

Signed-off-by: binoychitale <binoychitale@gmail.com>

Refactor common arg parsing into a separate method

bbdc2db

Signed-off-by: binoychitale <binoychitale@gmail.com>

Cargo fmt fixes

2167549

Signed-off-by: binoychitale <binoychitale@gmail.com>

binoychitale force-pushed the cli-client branch from dc27244 to 2167549 Compare January 29, 2021 18:05

ysimonson reviewed Jan 30, 2021

View reviewed changes

ysimonson mentioned this pull request Jan 30, 2021

Batch import from CSV via client #161

Open

binoychitale added 3 commits January 31, 2021 18:28

Refactor transaction instance creation

2b1672c

Signed-off-by: binoychitale <binoychitale@gmail.com>

Move invalid vertex error out of lib

b684feb

Signed-off-by: binoychitale <binoychitale@gmail.com>

Cargo fmt fixes

c4819b4

Signed-off-by: binoychitale <binoychitale@gmail.com>

fix clippy warnings

bd06214

Signed-off-by: binoychitale <binoychitale@gmail.com>

ysimonson merged commit a704385 into indradb:master Feb 2, 2021

ysimonson mentioned this pull request Feb 2, 2021

Bundle client in releases #163

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cli client #157

Cli client #157

binoychitale commented Jan 25, 2021

ysimonson Jan 25, 2021

ysimonson Jan 25, 2021

ysimonson Jan 25, 2021 •

edited

binoychitale Jan 26, 2021

binoychitale Jan 26, 2021

ysimonson Jan 26, 2021

ysimonson Jan 26, 2021

ysimonson Jan 26, 2021

ysimonson Jan 26, 2021

ysimonson Jan 26, 2021

ysimonson Jan 26, 2021

binoychitale commented Jan 27, 2021

ysimonson commented Jan 27, 2021

ysimonson commented Jan 28, 2021

ysimonson commented Jan 29, 2021

binoychitale commented Jan 29, 2021

ysimonson commented Jan 29, 2021

binoychitale commented Jan 29, 2021

ysimonson Jan 30, 2021

ysimonson Jan 30, 2021

binoychitale Jan 31, 2021

ysimonson Jan 31, 2021

ysimonson Jan 30, 2021

binoychitale Jan 31, 2021

ysimonson Jan 31, 2021

ysimonson Jan 30, 2021

ysimonson Jan 30, 2021

ysimonson commented Feb 1, 2021

binoychitale commented Feb 2, 2021

ysimonson commented Feb 13, 2021

		@@ -160,9 +163,11 @@ pub async fn main() -> Result<(), Box<dyn Error>> {
		.get_matches();

		let _address = matches.value_of("address").unwrap();

Cli client #157

Cli client #157

Conversation

binoychitale commented Jan 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ysimonson Jan 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

binoychitale commented Jan 27, 2021

ysimonson commented Jan 27, 2021

ysimonson commented Jan 28, 2021

ysimonson commented Jan 29, 2021

binoychitale commented Jan 29, 2021

ysimonson commented Jan 29, 2021

binoychitale commented Jan 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ysimonson commented Feb 1, 2021

binoychitale commented Feb 2, 2021

ysimonson commented Feb 13, 2021

ysimonson Jan 25, 2021 •

edited