Add initial parsec-tool implementation #1

joechrisellis · 2020-07-22T13:39:46Z

Hi guys, this is the initial parsec-tool implementation from issue 202 in parallaxsecond/parsec.

Any and all comments welcome and appreciated! :)

ionut-arm

Thanks for seeding this!! I've left a few comments below.

Another question is how we test this. I would've said some pure CLI tests are in order - you could run them and redirect the output to two files (stdout and stderr) and them compare with some predefined values. Doesn't track the colours and it's a bit primitive. Alternatively, you can try and mock the client communication (like we do in the client tests) and use something like insta to check what you then print.

ionut-arm · 2020-07-22T13:56:13Z

src/output.rs

@@ -0,0 +1,112 @@
+// Copyright 2020 Contributors to the Parsec project.


Have you looked at libraries to handle the colour/style? Like mowl. Or did you want to avoid using log so as not to print anything but what you choose to, the way you want it to look?

I took a look around for some libraries to handle logging/style but I couldn't find anything that was as flexible as ansi_term, which is what I've opted for here. Logging felt a little bit extreme, since we're not really 'logging' as much as we are just writing to std{out,err}. Happy to change this if you think it's a better long term solution though!

No, this makes sense, it's not really logging in the end :) I think I was mostly bothered by the thought of having to set the colour for each line that we write, feels a bit extreme.

src/cli.rs

ionut-arm · 2020-07-22T14:13:13Z

src/command.rs

+/// Gets the list of availble opcodes for a given provider and outputs them.
+fn list_opcodes(
+    provider: &str,
+    _command_line_context: &CommandLineContext,


I'd say, for all these functions, if you don't need this argument you shouldn't really pass it in/request it

Sure, will remove.

Actually, I'm not so sure about this -- in the future it's likely that we'll want to show more/less information depending on the verbosity, which is encoded in the command line context. I think this is a good thing to keep around, even if we're not using it right now.

Makes sense

ionut-arm · 2020-07-22T14:17:57Z

src/command.rs

+        print_colored!(Cyan, "0x{:02x} ({})", provider.id as u32, provider.id);
+        println!("]");
+
+        print_colored!(Yellow, "Description");


If you keep this system for colouring output, you should probably write down somewhere what each colour represents (for example, why the details here are yellow - I would've assumed yellow is more of a warning colour).

I'm not saying yellow is wrong, btw, just that it should be clear what the colours are used for

That's a good idea -- so far, the format is:

Yellow to represent field names -- i.e. 'description', 'uuid'.

Cyan for titles.

Blue for information.

Red for errors (and warnings?).

Green for success messages.

If we agree on a list like this, would it make sense to make separate macros for each of these to handle both the colour and the printing inside the macro? So for example, instead of

print_colored!(Yellow, "Description"); println!(": {}", provider.description);

you'd get the same result via

print_field!("Description", "{}", provider.description);

Apologies if this is a bit over the top 😅

src/output.rs

hug-dev

Thanks a lot for setting up the bases of the Parsec Tool, that will be very helpful.

I left some comments below. Generally, I can see that you are using the BasicClient from the Parsec Client, which is the obvious, straightforward choice. I was thinking though if using the OperationClient could be a good idea as well. If we find ways to:

generate the correct commands, arguments and text from the Operation structures (maybe using procedural macros)
format and print the Result structures in the same structured way, with a color scheme (similar to what you have done)

then we can simply call the OperationClient to execute the operation and it would be very easy to add new operations to the tool in the future, with very little work.
What do you think? I am happy to merge this PR with your current idea but tackle this one in a next one.

hug-dev · 2020-07-28T18:36:51Z

Cargo.toml

+ansi_term = "0.12"
+anyhow = "1.0.31"
+atty = "0.2"
+clap = "3.0.0-beta.1"
+parsec-client = "0.7.1"
+thiserror = "1.0"
+


Can you please also add those dependencies in the README.md file? You can take example of our other repos.

hug-dev · 2020-07-29T08:57:29Z

src/main.rs

@@ -33,6 +33,26 @@

 //! Parsec Tool: a tool to communicate with Parsec from the command-line

+#[macro_use]


Is it possible to use:

use output::e_err;

instead of macro_use attribute? I think it is clearer to spot which macros come where as for static functions.

I don't think so -- the purpose of this line is to make explicitly make the output module available in the crate. If we want to use its macros, I think macro_use is necessary. 🙁

What about something like:

mod output; use output::e_err;

?

hug-dev · 2020-07-29T08:58:59Z

src/main.rs

+mod output;
+
+mod cli;
+mod command;
+mod common;
+mod error;


Up to discussion, but I think it would be better to export those modules from a src/lib.rs file. That way documentation can be generated for those on docs.rs.

Do you intend that documentation for users of the tool or for its developers?

For developpers only, the library crate exposed should probably not be used.

hug-dev · 2020-07-29T09:05:00Z

src/main.rs

+use crate::command::dispatch_command;
+use anyhow::{Context, Result};
+
+fn run() -> Result<bool> {


Is the bool result actually used? If not, this function can maybe return Result<()>.

No, not yet. The idea was going to be:

Return Err on complete failure.

Return Ok(true) on success with partial failure.

Return Ok(false) on complete success.

But since we aren't using this right now, I agree -- better to just remove it.

hug-dev · 2020-07-29T09:10:35Z

src/cli.rs

+                Arg::with_name("provider")
+                    .short('p')
+                    .takes_value(true)
+                    .required(true)


Could the p argument be not required and if not passed, the list_opcodes command would execute on the default provider (the first one returned by list_providers)?

Good idea, will change this.

hug-dev · 2020-07-29T09:14:08Z

src/cli.rs

+        } else {
+            unreachable!()
+        },
+        app_name: match matches.value_of("appname") {


Is appname one of the possible option? I don't see it in matches

hug-dev · 2020-07-29T09:18:42Z

src/cli.rs

+use clap::{App, AppSettings, Arg};
+
+#[derive(Debug)]
+pub enum Action {


In the future, Action should probably be replaced by NativeOperation to not have to duplicate similar information. This module would create the correct instance of NativeOperation from the command entered by the user.
Having an automatic translation (as a procedural macro maybe?) of each of the Operation into command line arguments for clap would be phenomenal.

Having an automatic translation (as a procedural macro maybe?) of each of the Operation into command line arguments for clap would be phenomenal.

I don't think that's a good idea for some of the operations at least, especially the ones that take very rich data structures. Imagine having to write the whole KeyAttributes structure on the command line. It's probably more sensible to expect stuff like that to be written in a TOML file or something similar, and we parse it from there.

Hence, I'm not sure just automating the whole translation process makes that much sense, unless we can specify, for example, which parameters of a command should be taken from files, and how the file should be converted to the parameter in the first place. Even for "very simple" data like a file that you want to encrypt, it's probably more reasonable to let the user decide if they want to pipe the contents through the CLI or have an option to select the file based on its path (or potentially write a signature for it in a different file, for example, instead of just blindly writing stuff to stdout).

That's true, I did not think about KeyAttributes, that would be very painful to write all down. Maybe more generally the CLI should limit itself to the highest abstraction level of the client? When we have features for parameters discovery and simple abstraction over complicated operations, then the CLI can have commands over that.

hug-dev · 2020-07-29T09:38:23Z

src/output.rs

+// - Cyan should be used for section titles.
+// - Yellow should represent field names -- i.e. UUIDs, versions, etc.
+//
+// By sticking to this guideline we ensure that the CLI stays consistent.


Agree with Ionut that it would be nice to have macros mapping with each one of those colours to be consistent everywhere. Also, it would maybe be nice to not have the macro_export but let the module where they are needed import them directly like static functions.

hug-dev · 2020-07-29T09:39:34Z

src/output.rs

+
+/// Write an info message to stdout.
+#[macro_export]
+macro_rules! info {


Is the verbosity level argument coming into play for the info, warn and err macros? Would it be possible to use a logger like env_var directly? I think it also handles colors.

joechrisellis · 2020-07-30T10:50:54Z

Hi guys. I've addressed your comments as well as done some further work on this. The initial implementation I have now is quite different to what is shown in this patch. Do you want me to close this PR and open a new one or would you rather I just push the new stuff here?

ionut-arm · 2020-07-30T10:55:25Z

Hi guys. I've addressed your comments as well as done some further work on this. The initial implementation I have now is quite different to what is shown in this patch. Do you want me to close this PR and open a new one or would you rather I just push the new stuff here?

Push it over this one, if someone asks why some things are the way they are, we can point them to this PR and it'll have the full convo.

hug-dev · 2020-07-30T13:28:39Z

Is everything changing in the new version? Would it make sense for you to first apply the changes requested here, merge this PR and then open a new one with the new changes?

Maybe it does not make sense if the comments we made are not relevent anymore.

(saying that so that it might be easier to review)

joechrisellis · 2020-07-30T14:31:53Z

@hug-dev a lot has changed -- I think I will just re-push the code here just to keep the conversation going. 🙂

ionut-arm

Thanks for the hard work on rewriting this! I really like the new structuring, it's quite smooth 😎 I did have some comments on how the components link together

ionut-arm · 2020-07-31T13:07:22Z

src/main.rs

+    dispatch_subcommand(matches).context("Executing subcommand failed.")?;
+    Ok(())


You can shorten this to

dispatch_subcommand(matches).context("Executing subcommand failed.")

Good spot! I think I'll leave this as-is, though -- to me, the more explicit use of Ok(()) is more readable. If you have strong opinions in this I'm happy to change though. 😄

ionut-arm · 2020-07-31T13:28:09Z

src/subcommands/list_providers.rs

+#[structopt(name = "list_providers")]
+pub struct ListProvidersSubcommand {}
+
+impl TryInto<NativeOperation> for ListProvidersSubcommand {


This'll be a bit of a churn, but instead of TryInto you should implement TryFrom (same for From and Into - you should generally implement only the first one directly). From the TryInto docs:

Library authors should usually not directly implement this trait, but should prefer implementing the TryFrom trait, which offers greater flexibility and provides an equivalent TryInto implementation for free, thanks to a blanket implementation in the standard library. For more information on this, see the documentation for Into.

(This is not to say that the pub trait ParsecToolSubcommand: StructOpt + TryInto<NativeOperation> should change, that one definitely makes sense)

Done, good spot!

ionut-arm · 2020-07-31T13:50:47Z

src/subcommands/common.rs

+/// effectively a `FromStr` implementation for AuthenticationData. Passing in `None` will return
+/// AuthenticationData::None. Passing in `Some(s)` will give you an app identity whose secret is
+/// built from the string `s`.
+pub fn make_authentication_data(auth_secret: Option<String>) -> AuthenticationData {


I think this can be implemented as a method on ParsecToolApp - you're not expecting to have to parse some other value as an AuthenticationData entity, right?

Quite right! Done. :)

ionut-arm · 2020-07-31T14:08:23Z

src/subcommands/mod.rs

+
+/// Runs the appropriate command given `matches`, which corresponds to the command-line invocation
+/// from the user.
+pub fn dispatch_subcommand(matches: ParsecToolApp) -> Result<(), ParsecToolError> {


Similar to the previous comment, this can be implemented as a method on Subcommand, or maybe even on ParsecToolApp, depending on how you want to call it in main().

We've generally avoided having "hanging" functions if we can implement them on some type where it makes sense for them to be attached. Sometimes that's not possible, though, either because the types are external so we literally cannot implement on them, or because we need multiple implementations for different things, so we then use functions.

ionut-arm · 2020-07-31T14:22:29Z

src/subcommands/mod.rs

+/// - They implement `run`, which executes the subcommand.
+pub trait ParsecToolSubcommand: StructOpt + TryInto<NativeOperation> {
+    /// Run the subcommand.
+    fn run(self, matches: ParsecToolApp) -> Result<(), ParsecToolError>;


I'd probably split that ParsecToolApp into Subcommand and Parameters or Options or something like that, where you group all the other parts. And then in this method you pass a reference to that other struct, it feels fishy to give to a subcommand ownership over the structure that presumably holds it? I'm assuming this works because Subcommand has Copy on it

ionut-arm

LGTM! 👌

hug-dev · 2020-08-04T14:48:02Z

src/cli/mod.rs

+
+/// Struct representing the command-line interface of parsec-tool.
+#[derive(Debug, StructOpt)]
+#[structopt(name=PROJECT_NAME, about=PROJECT_DESC, author=PROJECT_AUTHOR, version=PROJECT_VERSION)]


Does structopt not use these fields from the Cargo.toml by default?

It does, but doesn't seem to do it for the authors, so it makes sense to just leave them all there IMO. 🙂

hug-dev · 2020-08-04T14:56:54Z

src/lib.rs

+#![deny(
+    nonstandard_style,
+    const_err,
+    // dead_code,


Why is this one commented? And unused as well?

That's a debugging artifact, my bad.

hug-dev · 2020-08-05T13:44:52Z

src/cli/mod.rs

+    /// effectively a `FromStr` implementation for AuthenticationData. Passing in `None` will return
+    /// AuthenticationData::None. Passing in `Some(s)` will give you an app identity whose secret is
+    /// built from the string `s`.
+    pub fn make_authentication_data(&self) -> AuthenticationData {


In the end this is similar to a getter, right? Doing also a conversion, maybe it could be named authentication_data directly?

Good idea, done.

hug-dev · 2020-08-05T13:58:35Z

src/error.rs

+//! Error definitions/handling.
+
+use parsec_interface::operations::NativeResult;
+use thiserror::Error;


That's neat, the thiserror crate 👍

It is! 😄

hug-dev · 2020-08-05T14:00:46Z

src/lib.rs

+pub mod cli;
+pub mod common;
+pub mod error;
+pub mod subcommands;


I like the tree structure you choosed, with subcommands as a module!

hug-dev · 2020-08-05T14:05:49Z

src/subcommands/list_opcodes.rs

+impl TryFrom<ListOpcodesSubcommand> for NativeOperation {
+    type Error = ParsecToolError;
+
+    fn try_from(list_opcodes_subcommand: ListOpcodesSubcommand) -> Result<Self, Self::Error> {
+        // Trivially converted to a `NativeOperation`.
+        Ok(NativeOperation::ListOpcodes(list_opcodes::Operation {
+            provider_id: ProviderID::try_from(list_opcodes_subcommand.provider_opts.provider)?,
+        }))
+    }
+}


Nice! It's a very good idea to use the OperationClient here. Also each command will make its own choice of either using the basic client or the operation client 👌

hug-dev · 2020-08-05T14:10:52Z

src/cli/mod.rs

+pub struct ParsecToolApp {
+    /// How verbose should we be?
+    #[structopt(short = "v", multiple = true, default_value = "0")]
+    pub verbosity: u8,


This does not seem to be used in the PR, are you going to implement it later?

Yes, I was thinking that verbosity would be useful to control the level of output for some of the more complex operations in the future. It's unused at the moment though. 🙂

hug-dev · 2020-08-05T14:15:12Z

src/util/output.rs

+macro_rules! info {
+    ($($arg:tt)*) => {
+        $crate::print_colored!(Blue, "[INFO] ");
+        println!($($arg)*);
+    };
+}


I think we should only keep one version of the macros, choosing only to emit logs to stdout or stderr. I would say that here we can simply log everything to stderr. If we want to have the choice, it might be best to have a configuration option to choose between stdout or stderr but for simplicity I think it's best to just log everything to stderr.

On that note, maybe we can delete those macros and directly use the one from log with the env_logger implementor? The verbosity flag could set the log level and I am pretty sure that this crate handles colouring as well 💯 It's also customizable to make it look nice for the command line.

Okay, I'll get rid of the stdout stuff!

I think using the env_logger module is a good idea, but I think I'll implement this in a future commit if that's alright with you? It might require some further change throughout this repo. 😛

Sure that's fine!

Signed-off-by: Joe Ellis <[email protected]>

hug-dev · 2020-08-05T16:41:37Z

src/cli/mod.rs

+
+impl ParsecToolApp {
+    /// Run the requested subcommand.
+    pub fn dispatch_subcommand(&self) -> Result<(), ParsecToolError> {


Nitpicking here but in a future version, if that is ok, Subcommand itself could implement the ParsecToolSubcommand trait so that you could call run on it directly from main without needing this method.

joechrisellis force-pushed the master branch from d3a2dd7 to 024a244 Compare July 22, 2020 13:43

joechrisellis mentioned this pull request Jul 22, 2020

Create a Parsec Command Line Interface Client parallaxsecond/parsec#202

Closed

hug-dev assigned joechrisellis Jul 22, 2020

hug-dev requested review from hug-dev and ionut-arm July 22, 2020 13:45

hug-dev added the enhancement New feature or request label Jul 22, 2020

ionut-arm reviewed Jul 22, 2020

View reviewed changes

joechrisellis force-pushed the master branch from 024a244 to a952477 Compare July 22, 2020 14:51

joechrisellis marked this pull request as draft July 22, 2020 14:54

joechrisellis force-pushed the master branch from a952477 to d8ea69f Compare July 28, 2020 10:16

hug-dev reviewed Jul 29, 2020

View reviewed changes

joechrisellis force-pushed the master branch 4 times, most recently from 3ebef14 to 895402a Compare July 30, 2020 15:32

ionut-arm reviewed Jul 31, 2020

View reviewed changes

joechrisellis force-pushed the master branch 2 times, most recently from 0874cdd to fa54c1d Compare August 3, 2020 16:49

ionut-arm approved these changes Aug 4, 2020

View reviewed changes

joechrisellis marked this pull request as ready for review August 4, 2020 10:39

hug-dev reviewed Aug 5, 2020

View reviewed changes

Add initial parsec-tool implementation

a0a9ebd

Signed-off-by: Joe Ellis <[email protected]>

joechrisellis force-pushed the master branch from fa54c1d to a0a9ebd Compare August 5, 2020 16:27

hug-dev reviewed Aug 5, 2020

View reviewed changes

hug-dev approved these changes Aug 5, 2020

View reviewed changes

hug-dev merged commit 6c562b2 into parallaxsecond:master Aug 5, 2020

		@@ -0,0 +1,112 @@
		// Copyright 2020 Contributors to the Parsec project.

		@@ -33,6 +33,26 @@

		//! Parsec Tool: a tool to communicate with Parsec from the command-line

		#[macro_use]

		dispatch_subcommand(matches).context("Executing subcommand failed.")?;
		Ok(())

Add initial parsec-tool implementation #1

Add initial parsec-tool implementation #1

Conversation

joechrisellis commented Jul 22, 2020

ionut-arm left a 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

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

hug-dev left a 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

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

ionut-arm Jul 29, 2020 • edited Loading

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

joechrisellis commented Jul 30, 2020 • edited Loading

ionut-arm commented Jul 30, 2020

hug-dev commented Jul 30, 2020 • edited Loading

joechrisellis commented Jul 30, 2020

ionut-arm left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ionut-arm left a 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

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

Choose a reason for hiding this comment

ionut-arm Jul 29, 2020 •

edited

Loading

joechrisellis commented Jul 30, 2020 •

edited

Loading

hug-dev commented Jul 30, 2020 •

edited

Loading