Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(bin/oli): use clap_derive to reduce boilerplate code #5233

Merged
merged 4 commits into from
Oct 23, 2024

Conversation

koushiro
Copy link
Member

Which issue does this PR close?

Closes #.

Rationale for this change

reduce boilerplate code

What changes are included in this PR?

use clap_derive (instead of clap_builder) to reduce boilerplate code

Are there any user-facing changes?

No

@koushiro
Copy link
Member Author

koushiro commented Oct 23, 2024

oli's tests are not currently checked in CI. Do we need to add them to CI?

➜  oli git:(use-clap-derive) ✗ cargo test --release --all-features
   Compiling oli v0.41.12 (/Users/qinxuan/Code/rust-lang/opendal/bin/oli)
    Finished `release` profile [optimized] target(s) in 37.08s
     Running unittests src/lib.rs (target/release/deps/oli-c04c0e204779fc76)

running 7 tests
test config::tests::test_load_from_env ... ok
test config::tests::test_parse_s3_location3 ... ok
test config::tests::test_parse_fs_location ... ok
test config::tests::test_load_from_toml ... ok
test config::tests::test_load_config_from_file_and_env ... ok
test config::tests::test_parse_s3_location ... ok
test config::tests::test_parse_s3_location2 ... ok

test result: ok. 7 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running unittests src/bin/oli.rs (target/release/deps/oli-139e3cb8d6e37d4c)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests/cat.rs (target/release/deps/cat-7fa8be95eddce583)

running 2 tests
test test_cat_for_path_in_current_dir ... ok
test test_basic_cat ... ok

test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.51s

     Running tests/cp.rs (target/release/deps/cp-f883dfcf27676c1a)

running 2 tests
test test_basic_cp ... ok
test test_cp_for_path_in_current_dir ... ok

test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.03s

     Running tests/ls.rs (target/release/deps/ls-c1a38755bc9e6f86)

running 1 test
test test_basic_ls ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s

     Running tests/rm.rs (target/release/deps/rm-071bb9fe37b27c4c)

running 2 tests
test test_basic_rm ... ok
test test_rm_for_path_in_current_dir ... ok

test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s

     Running tests/stat.rs (target/release/deps/stat-ca4edaf8b3ac28e0)

running 2 tests
test test_stat_for_path_in_current_dir ... ok
test test_basic_stat ... ok

test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s

   Doc-tests oli

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

@Xuanwo
Copy link
Member

Xuanwo commented Oct 23, 2024

Hi, I implemented this to support the use of a proxy as follows:

ln oli ocp
ocp <src> <dst>

ln oli ols
ols <src>

Users can use ocp in exactly the same way as oli cp.

After this change, can we still operate in this manner?

@koushiro
Copy link
Member Author

koushiro commented Oct 23, 2024

Hi, I implemented this to support the use of a proxy as follows:

ln oli ocp
ocp <src> <dst>

ln oli ols
ols <src>

Users can use ocp in exactly the same way as oli cp.

After this change, can we still operate in this manner?

No, but I can try to support this manner.
Do you think the other parts are ok?

@Xuanwo
Copy link
Member

Xuanwo commented Oct 23, 2024

Do you think the other parts are ok?

Other parts looks good to me, thank you for working on this!

@koushiro
Copy link
Member Author

koushiro commented Oct 23, 2024

Hi, I implemented this to support the use of a proxy as follows:

ln oli ocp
ocp <src> <dst>

ln oli ols
ols <src>

Users can use ocp in exactly the same way as oli cp.

I encountered two problems:

  1. The previous (before this PR) implementation had the following problem:
➜  oli git:(main) ln target/release/oli ols
➜  oli git:(main) ✗ ./ols src
error: 'ols' requires a subcommand but one was not provided

Usage: ols [OPTIONS] <target>

For more information, try '--help'.
  1. If we use clap_derive and want to support the proxy manner that you mentioned, then we cannot use the global default config arg, and we need to add a default config for each cmd, can you accept this implementation?
./target/release/oli -h
OpenDAL Command Line Interface

Usage: oli <COMMAND>

Commands:
  cat   Display object content
  cp    Copy object
  ls    List object
  rm    Remove object
  stat  Show object metadata
  help  Print this message or the help of the given subcommand(s)

Options:
  -h, --help     Print help
  -V, --version  Print version
➜  oli git:(use-clap-derive) ✗ ln target/release/oli ols
➜  oli git:(use-clap-derive) ✗ ./ols -h
List object

Usage: ols [OPTIONS] <TARGET>

Arguments:
  <TARGET>

Options:
      --config <CONFIG>  Path to the config file [default: "/Users/qinxuan/Library/Application Support/oli/config.toml"]
  -r, --recursive        List objects recursively
  -h, --help             Print help
➜  oli git:(use-clap-derive) ✗ ./ols src
src/
➜  oli git:(use-clap-derive) ✗ ln target/release/oli ocat
➜  oli git:(use-clap-derive) ✗ ./ocat -h
Display object content

Usage: ocat [OPTIONS] <TARGET>

Arguments:
  <TARGET>

Options:
      --config <CONFIG>  Path to the config file [default: "/Users/qinxuan/Library/Application Support/oli/config.toml"]
  -h, --help             Print help
➜  oli git:(use-clap-derive) ✗ ./ocat DEPENDENCIES.md
# Dependencies

Refer to [DEPENDENCIES.rust.tsv](DEPENDENCIES.rust.tsv) for full list.

@koushiro
Copy link
Member Author

koushiro commented Oct 23, 2024

I add a new type ConfigParams in b7b0642 to reduce the same argument declaration, or you think it's not necessary?

@Xuanwo
Copy link
Member

Xuanwo commented Oct 23, 2024

2. If we use clap_derive and want to support the proxy manner that you mentioned, then we cannot use the global default config arg, and we need to add a default config for each cmd, can you accept this implementation?

Hi, that's look to me.

@Xuanwo
Copy link
Member

Xuanwo commented Oct 23, 2024

I add a new type ConfigArgs (or ConfigParams?) in 3808562 to reduce the same argument declaration, or you think it's not necessary?

Make sense to me.

@koushiro
Copy link
Member Author

@Xuanwo All done, PTAL
BTW, do we need to add oli's tests to CI or just keep it.

@Xuanwo
Copy link
Member

Xuanwo commented Oct 23, 2024

BTW, do we need to add oli's tests to CI or just keep it.

Including an oli test in the CI would be appreciated.

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thank you @koushiro for this!

@Xuanwo Xuanwo merged commit c230c8a into apache:main Oct 23, 2024
12 checks passed
@koushiro koushiro deleted the use-clap-derive branch October 23, 2024 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants