Refactor args #805

chevdor · 2023-01-27T13:53:16Z

replace File and Url by an enum: Source
allow Source to be passed as ENV

- move url to a global arg - allow url to be passed as ENV - use clap for conflicting args

jsdw · 2023-01-27T13:57:43Z

cli/src/main.rs

+        default_value = "http://localhost:9933",
+        env = "SUBXT_URL"
+    )]
+    url: Uri,


I don't like this being moved to be a global arg; yes, perhaps at the moment eac hsubcommand requires a URL, but that definitely need not always be the case (eg why should a version commad need a url).

I'd much rather that each command specifies every option it cares about, and I don't care if that happens to lead to some code duplication because I think it also makes it easier to maintain :)

(something that you can do I think is to have a custom Uri type that has these clap attrs on and such so that you can just use the custom type in each command where we want it. I'd prefer that sort of thing (assuming it's actually possible as I described it)

Sure not all the commands may need a url but it is unlikely that the core of subxt will not work without a URL. With the URL as part of each command, it is rather hard to inject the URL when it is not the default.

Making the URL global makes it available to all commands, they don't have to use it.

It makes the experience for users and devs also nicer as you can define the ENV once and then keep using commands without specifying the --url in various places.

Even without the ENV, it allows using a command that always start with subxt --url <foobar> <the command> and users can easily recall their command history and run another command by changing only the command part.

Won't the ENV var part work if the variable is part of some subcommand? (and in that case, if somebody always wants to have some URL set then they can just set the ENV var and not worry about the --url at all).

And I don't feel that being able to put --url first is worth it anyway (also, iirc it also requires that URL comes first, unlike all other params which must follow the subcommand, and I don't like that either; it's not consistent).

So I am not convinced I'm afraid!

I am with you for things like file since most of the commands do not need a file (if that last assumption is wrong, then file should be IMO global as well). I am basing this PR on the fact that subxt is a tool that mostly will need to connect to an endpoint (besides a few utility commands such as version, help, etc..).

If the args are local to the commands, you cannot call the following: subxt --url <foo> <command_1>
and you must pass: subxt <command_1> --url <foo>.

Having --url global however allows both and the support as ENV make the flag vanish from the user's view.

cargo for instance has lots of global level ENV such as RUSTDOCFLAGS that is totally irrelevant to some of the cargo commands such as cargo --version and if the user would happen to provide the flags or have the ENV set, this is simply ignored.

jsdw · 2023-01-27T13:59:23Z

cli/src/main.rs

-        Command::Compatibility(opts) => commands::compatibility::run(opts).await,
-        Command::Version(opts) => commands::version::run(opts),
+    match &opts.subcmd {
+        SubCommand::Metadata(cmd_opts) => commands::metadata::run(&opts, &cmd_opts).await,


Also you end up with this sort of thing which I find a bit weird; commands::metadata::run(&opts, &cmd_opts) is now taking in all of the opts, but it's also taking in part of the same thing again, the SubCommand opts.

Yes. I think this is a trade off and also you sharing some global flags (--json is another typical one) with all the commands. Sure there are cases where it is non-sense (ie subxst --url <bla> version) but that would nonetheless work and not get in the way of the user.

Are you concerned about the amount of extra data we pass ?

Oh nothing like that. For me, I think that in general, moving out the url from each command reduces the clarity and maintainability of the code. It's very simple to understand at present that for each command you'd like to add/work on, you can specify exactly the options that it wants and you can see all options specified in the same file as the command logic.

I don't want us to go down a path that will result in "global" options eg url (because most commands use them) and then the extra muddle in the code that this introduces. For me it's just not worth it. What I am much more in favour of is things like helper structs which encompass "special URL command logic" that we can use to easily gain that URL logic in any command we want to use it for.

I understand your logic but I respectfully disagree for the following reasons:

code and doc duplication: We can see the drawback currently with the doc of the codegen's url flag that mentions the metadata command, it is probably the remain of a copy/paste

it suggests that the url for the metadata command does not have the same meaning than the url of the codegen command. I would be in favor of keeping --url local if the url would be pointing to different things but for subxt, they point to the endpoint url AFAIK

I don't know all the plans for subxt and if it is not aimed at becoming primarily a tool to send transactions or query a node reached via the url of its endpoint, then I would agree that keeping things local and dealing with the duplication'costs could be worth it. But I think subxt is primarily targetted at connecting to a node, even if some utility commands may be part of it and not require a url.

If you plan on adding later for instance a --json flag so all outputs are automation friendly, it is MUCH easier to have that also global that spreading dupllication is most or all commands. That also means you'd need to add this --json flag (or --url) in every single upcoming new command, which is quite some hassle for a small gain.

jsdw · 2023-01-27T15:00:11Z

While I am not in favour of "globalising" args like URL, I do agree that if we are repeating the same arg in multiple commands (and it has the same meaning), then we can do better. Eventually it might be nice to factor out some repeated commands like so:

// opts for some subcommand
#[derive(Debug, ClapParser)]
pub struct Opts {
    /// The url/path of the substrate node to query for metadata for codegen.
    #[clap(flatten)]
    metadata_opts: MetadataOpts,
    // ... other opts
}

// This custom struct can live in some general location and be reused anywhere we want to be obtaining metadata 
#[derive(Debug, Clone, clap::Args)]
pub struct MetadataOpts {
    /// The url of the substrate node to query for metadata.
    #[clap(name = "url", long, value_parser, env = "SUBXT_METADATA_URL", conflicts_with = "file")]
    url: Option<Uri>,
    /// The path to the encoded metadata file.
    #[clap(short, long, value_parser, env = "SUBXT_METADATA_PATH", conflicts_with = "url")]
    file: Option<PathBuf>,
}

impl MetadataOpts {
    // perhaps this even has a helper fn or whatever..
    fn fetch_metadata_bytes(&self) -> Result<Vec<u8>, ...> { ... }
}

Maybe at some point we want to submit transactions and then we have just a URL that's needed (and can be some shared thing to be consistent toomaybe). Currently we are only interested in obtaining metadata, and so actually "url"/"file" do sortof live together and if they were consistent across commands it'd be nice.

Regardless of whether it's a top level or subcommand, env = "SUBXT_METADATA_URL" works well though, so with thecurrent approach + that env thing you have the benefit that all --command's are declared in a consistent location (after the subcommand) and if you want to avoid typing it a lot you just set the env var once.

Ultimately I think that is the best place to be. I don't want to start making options global.

chevdor · 2023-01-27T15:26:33Z

Ok, I was about to bring a short-coming of the current PR related to this issue before reading your comment.

I removed a piece of code to let clap handles the conflicts between file and url but with url global and file local, we hit the bug mentioned above. A workaround if url is global and file is local is to let the conflict happen but consider that file, if defined, overwrites the global url and file will be used. That can be made clear to the user with a warning.

This is a non issue if we bring --file as global as well :)
Or the check I removed needs to be temporaily brought back until the clap issue is fixed.

"url"/"file" do sortof live together and if they were consistent across commands it'd be nice

and here comes the problem if you keep them local, each new command will have to use this code to say that file and url are exclusive. It can be refactored into a call defined in one place but this one call feel un-necessary and will be required for each new call :/

Using both args as global makes it really trivial and we let clap deal with args, default and conflicts, this is what it is meant to do and makes the code also simpler as you no longer get a Option<Uri> but a Uri since clap caught the issues before we got there.

Besides the global vs local topic, there is another elegant option which is to merge both file and url into a new source (or whatever name) that is a Rust enum.
We can then impl From<String> for Source. Urls can be "spotted" easily, then what is not a URL can be considered as a file-maybe, after a check on the FS, we can conclude whether or not this is a valid file.

Regardless of whether it's a top level or subcommand, env = "SUBXT_METADATA_URL" works well though, so with thecurrent approach + that env thing you have the benefit that all --command's are declared in a consistent location (after the subcommand) and if you want to avoid typing it a lot you just set the env var once.

Yes I agree. The 2 topics global vs local and using ENV are not related. Using local variable based on the ENV would already go into the right direction.

chevdor · 2023-01-27T15:44:56Z

To summarize the options I see (leaving aside the ENV topic we agree on):

1: url + file remain 2 args and promoted global, conflicting args can be dealt with with clap
2: url + file remain both local (and ENV helps), conflicting args can be dealt with with clap
3: url + file merged into an enum and promoted global, no conflict possible
4: url + file merged into an enum and remain local, no conflict possible

I excluded the unbalanced option (such as url being global and file local) as it feels a little odd.

jsdw · 2023-01-27T15:52:32Z

I will accept a PR for 4 (2 is already true), but won't go for any form of global args, my reasons against are summarised as:

The code becomes less clean with global args (imo).
Inconsistent way of providing args (must use subxt --a subcmd --b --c with global args instead of the more consistent subxt subcmd --a --b --c).
URL is already not needed for every command. I don't want to assume that it will be in the future either.
ENV vars solve the (very minor) issue of avoiding retyping the URL or whatever.

Overall this is very much bikeshedding IMO given how small the cli tool is and not something I want to be spending much time thinking on :)

chevdor · 2023-01-27T17:01:12Z

OK, I will make changes towards option 4. then.

Note however that the following is not correct:

Inconsistent way of providing args (must use subxt --a subcmd --b --c with global args instead of the more consistent subxt subcmd --a --b --c)

with --a global the user can provide any of:

subxt --a subcmd --b --c
subxt subcmd --a --b --c
subxt subcmd --b --c --a

jsdw · 2023-01-30T10:22:04Z

OK, I will make changes towards option 4. then.

Note however that the following is not correct:

Inconsistent way of providing args (must use subxt --a subcmd --b --c with global args instead of the more consistent subxt subcmd --a --b --c)

with --a global the user can provide any of:
* `subxt --a subcmd --b --c`

* `subxt subcmd --a --b --c`

* `subxt subcmd --b --c --a`

Ah that's cool; I geuss clap is better than structopt in the way it handles these then; StructOpt always had that issue :)

- add new Source enum for File | Url - move back Source arg to local args for the metadata and codegen commands

jsdw · 2023-02-01T16:14:27Z

README.md

+2. Set the URL you want to use, you may skip this step and the default `http://localhost:9933/` will be used:
+```
+export SUBXT_URL=<your endpoint>
+```


I prefer the original doc format (in other words; show the simplest steps here and mention below using --url or the new SUBXT_URL env var to set the URL).

jsdw · 2023-02-01T16:15:46Z

cli/src/main.rs

+        SubCommand::Metadata(cmd_opts) => commands::metadata::run(&opts, cmd_opts).await,
+        SubCommand::Codegen(cmd_opts) => commands::codegen::run(&opts, cmd_opts).await,
+        SubCommand::Compatibility(cmd_opts) => {
+            commands::compatibility::run(&opts, cmd_opts).await
+        }
+        SubCommand::Version(cmd_opts) => commands::version::run(&opts, cmd_opts),


Can we stop passing &opts to the run commands now since it's pointless :)

jsdw · 2023-02-01T16:16:25Z

cli/src/main.rs

+pub enum SubCommand {
+    Metadata(commands::metadata::MetadataOpts),
+    Codegen(commands::codegen::CodegenOpts),
+    Compatibility(commands::compatibility::CompatOpts),
+    Version(commands::version::Version),
+}
+
+#[derive(Debug, Parser)]
+#[clap(version = crate_version!(), author = crate_authors!(), color=ColorChoice::Always)]
+pub struct CliOpts {
+    #[clap(subcommand)]
+    pub subcmd: SubCommand,


Is there any advantage to these being two structs and not one struct like before now?

jsdw · 2023-02-01T16:17:16Z

cli/src/main.rs

+}
+
+#[derive(Debug, Parser)]
+#[clap(version = crate_version!(), author = crate_authors!(), color=ColorChoice::Always)]


ColorChoice::Always may be annoying for this in CI scripts (ie lots of command codes emitted); perhaps best to leave it at the default?

jsdw · 2023-02-01T16:18:53Z

cli/src/commands/source.rs

+        match s {
+            s if (s.starts_with("ws://")
+                | s.starts_with("wss://")
+                | s.starts_with("http://")
+                | s.starts_with("https://"))
+                && s.parse::<Uri>().is_ok() =>
+            {
+                Ok(Source::Url(s.parse().unwrap()))
+            }
+            p if Path::new(p).exists() => Ok(Source::File(PathBuf::from(p))),
+            _ => Err(format!("File does not exist: {s}")),
+        }


I'd prefer to just stick with if over a match that is just mimicking if's :)

jsdw · 2023-02-01T16:19:03Z

cli/src/commands/source.rs

+pub enum Source {
+    File(PathBuf),
+    Url(Uri),
+}


Cool; like this!

jsdw · 2023-02-01T16:19:33Z

cli/src/commands/source.rs

@@ -0,0 +1,30 @@
+use std::path::{


source.rs isn't a command; it's some utility thing. Can we move it into a utils.rs or similar please?

I'd like to keep the convention that all files in commands are actually subcommands that the otol can run, for clarity.

jsdw · 2023-03-15T11:34:59Z

I'll close this for now (and can understand that it was a lot more back and forth than you probably wanted, so sorry about that!). It'd be nice to take the Source stuff and ENV_VAR support from this though at some point!

chevdor · 2023-03-15T12:59:19Z

Yeah no worry, I just did not have the bandwidth to allocate to that so far.

chevdor and others added 3 commits January 27, 2023 14:51

Refactor args

606bc3b

- move url to a global arg - allow url to be passed as ENV - use clap for conflicting args

Add doc

486e85d

fmt

f117fab

jsdw reviewed Jan 27, 2023

View reviewed changes

chevdor and others added 2 commits January 27, 2023 15:03

Fix string formatting

40ddf19

Merge branch 'master' into wk-230127-url-global-env

e7d16c3

chevdor and others added 3 commits January 31, 2023 10:09

Merge branch 'master' into wk-230127-url-global-env

2906370

Add Source enum

7a0963e

- add new Source enum for File | Url - move back Source arg to local args for the metadata and codegen commands

Better error handling

bde61a3

chevdor marked this pull request as ready for review February 1, 2023 09:13

chevdor added enhancement New feature or request rust Pull requests that update Rust code labels Feb 1, 2023

jsdw reviewed Feb 1, 2023

View reviewed changes

cli/src/commands/source.rs

Comment on lines +9 to +12

pub enum Source {

File(PathBuf),

Url(Uri),

}

Copy link

Collaborator

jsdw Feb 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool; like this!

jsdw reviewed Feb 1, 2023

View reviewed changes

jsdw closed this Mar 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor args #805

Refactor args #805

chevdor commented Jan 27, 2023 •

edited

Loading

jsdw Jan 27, 2023

jsdw Jan 27, 2023

chevdor Jan 27, 2023 •

edited

Loading

jsdw Jan 27, 2023

chevdor Jan 27, 2023

jsdw Jan 27, 2023

chevdor Jan 27, 2023

chevdor Jan 27, 2023

jsdw Jan 27, 2023

chevdor Jan 27, 2023

jsdw commented Jan 27, 2023

chevdor commented Jan 27, 2023

chevdor commented Jan 27, 2023

jsdw commented Jan 27, 2023

chevdor commented Jan 27, 2023

jsdw commented Jan 30, 2023

jsdw Feb 1, 2023

jsdw Feb 1, 2023

jsdw Feb 1, 2023

jsdw Feb 1, 2023 •

edited

Loading

jsdw Feb 1, 2023

jsdw Feb 1, 2023

jsdw Feb 1, 2023 •

edited

Loading

jsdw commented Mar 15, 2023

chevdor commented Mar 15, 2023

Refactor args #805

Refactor args #805

Conversation

chevdor commented Jan 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chevdor Jan 27, 2023 • 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

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

jsdw commented Jan 27, 2023

chevdor commented Jan 27, 2023

chevdor commented Jan 27, 2023

jsdw commented Jan 27, 2023

chevdor commented Jan 27, 2023

jsdw commented Jan 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw Feb 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw Feb 1, 2023 • edited Loading

Choose a reason for hiding this comment

jsdw commented Mar 15, 2023

chevdor commented Mar 15, 2023

chevdor commented Jan 27, 2023 •

edited

Loading

chevdor Jan 27, 2023 •

edited

Loading

jsdw Feb 1, 2023 •

edited

Loading

jsdw Feb 1, 2023 •

edited

Loading