Refactor into jcli-lib crate #3216
Conversation
This is the beginning step for refactoring the jcli-lib crate from the jcli binary crate. This is the starting commit for #3172
filip-dulic-bloxico
force-pushed
the
jcli-lib-refactor
branch
from
2f5f5a9
to
a28223f
Compare
filip-dulic-bloxico
changed the title
filip-dulic-bloxico
force-pushed
the
jcli-lib-refactor
branch
from
a3179ba
to
b5235e9
Compare
This module really doesn't make sense to have if the 'structopt' feature is disabled. The gated attributes can be safely removed as long as the entire module is gated. I'm leaving the gated attributes in place, until the API is better defined.
There was a problem hiding this comment.
The idea itself is good, BUT. IMO what is being done currently defies the whole purpose of moving functionality into the library. This library includes things that should be there (argument parsing) since they are going to be used only in jcli and this is not what I would call a good API either.
See my review comment below: instead of moving everything into the library we should start to gradually separate the functionality, that is needed to be shared to other apps.
I don't even mind not having the full functionality of jcli in the library in the first version, I understand this is a gigantic piece of work. This is perfectly fine as long as it is usable in other apps.
That said, instead of doing what we have done currently I would suggest to:
- Identify the current clients of this library (excluding jcli).
- Design a proper API that would not bring in the whole dependency tree of
jcli. Given the nature of this library I assume that this would be just a bunch functions.
| @@ -0,0 +1,19 @@ | |||
| fn main() { | |||
There was a problem hiding this comment.
This is not needed for the library actually. We should be pretty much happy with what Cargo.toml provides.
| use structopt::StructOpt; | ||
| use thiserror::Error; | ||
|
|
||
| #[derive(StructOpt)] | ||
| #[structopt(name = "address", rename_all = "kebab-case")] | ||
| #[cfg_attr( |
There was a problem hiding this comment.
I would oppose that. Instead just "move the functionality to the library and optional command parser" we should implement an API that is intended for library users. By this I mean that the current approach, that is initially intended for "parse arguments and run a command" is not suitable here. Most commands could (and should) be made into functions receiving several arguments.
Let's reserve argument parsing for the actual CLI binary and think of a better approach here.
There was a problem hiding this comment.
Actually, argument parsing could be reused too. Even inside jcli itself we have some duplicated code. Also some tools could reuse some of that code (it is being used for a tool now on catalyst-toolbox).
There was a problem hiding this comment.
Even inside jcli itself we have some duplicated code
This is the problem of a client, not the problem of a library. ;)
Also some tools could reuse some of that code
Need to look at the amount.
There was a problem hiding this comment.
This is the problem of a client, not the problem of a library
In this case, it is both.
There was a problem hiding this comment.
The library could implement things like parsing common argument formats. Probably not the options and subcommand structures though, these should be defined in the CLI tool.
+1
IMO we could make every (needed) module could have an IO part (args parsing that can be reusable, some of the modules already had it, but they were private), and extract and refactor the |
Sounds about right. I am still a bit skeptical about sharing |
|
I let it as a draft for now, while we discuss what we will finally do. |
danielSanchezQ
added
the
DO NOT MERGE
|
jcli is currently used as a library in catalyst-toolbox, so it would make sense to check what is it used for and provide an API. |
|
@filip-dulic-bloxico Closing it. Feel free to reopen if we really need it. |
This is the beginning step for refactoring the jcli-lib crate out of the
jcli binary crate.