Chores: makefile, tests #2
Conversation
Signed-off-by: Alexis Delain <quiet.syscall@proton.me>
Signed-off-by: Alexis Delain <quiet.syscall@proton.me>
Signed-off-by: Alexis Delain <quiet.syscall@proton.me>
Signed-off-by: Alexis Delain <quiet.syscall@proton.me>
nanoandrew4
left a comment
nanoandrew4
left a comment
There was a problem hiding this comment.
LGTM, seems like most changes are either formatting, tests or refactoring, with just a few actual code changes. Lets wait for someone with more Rust experience to review the few things that have changed in case they could be problematic.
|
|
||
| # The Kani Rust Verifier for checking safety of the code | ||
| kani: | ||
| @command -v kani >/dev/null || \ |
There was a problem hiding this comment.
I don't like this approach.
When a tool is instructed to do something, it should do something rather than installing dependencies. Any 'check' is a read-only operation that should never modify the system's state.
There was a problem hiding this comment.
My comment was not to open the discussion about that, it's to keep the same approach eveywhere:
https://github.com/hypernetix/rclib/pull/2/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52R8
We have a makefile function to install cargo tools, let's reuse that function
|
|
||
| # Generate code coverage report (HTML) | ||
| coverage: | ||
| @command -v cargo-llvm-cov >/dev/null || (echo "Installing cargo-llvm-cov..." && cargo install cargo-llvm-cov) |
|
|
||
| # Generate code coverage report (text) | ||
| coverage-text: | ||
| @command -v cargo-llvm-cov >/dev/null || (echo "Installing cargo-llvm-cov..." && cargo install cargo-llvm-cov) |
| .arg(Arg::new("timeout").long("timeout").short('t').help("Request timeout in seconds (after connection)").default_value("300").num_args(1)) | ||
| .arg(Arg::new("openapi-file").long("openapi-file").help("Path to OpenAPI spec file").num_args(1)) | ||
| .arg(Arg::new("mapping-file").long("mapping-file").help("Path to mapping YAML file").num_args(1)) | ||
| .arg( |
There was a problem hiding this comment.
Why it's not declared as a struct with the macro?
There was a problem hiding this comment.
From what I know this .arg chaining is idiomatic for clap
There was a problem hiding this comment.
By looking at the clap documentation:
https://docs.rs/clap/latest/clap/
There are two ways of developing a CLI, builder and derive pattern. Both use macros, the first as declarative macro and the second as derived macro.
The derived macro is more used as you defined your Cli struct with everything that it's required and you save a lot of testings, as a lot of the code is generated automatically with that.
The builder as it's implemented right now it uses &str that there's no other way of checking of the existence of flags, but to have tests that check for that.
Also, the use of leak_str: https://github.com/hypernetix/rclib/pull/2/files/3e936a881b711332bc9614df73e316c82b3b0c9d#diff-390790450d579f92eb671a71adc4ee01b59d3088b94fe2ad62c611622a4181a9R18 is not idiomatic. It's doing unnecesary tricks.
| [package] | ||
| name = "rclib" | ||
| version = "0.1.0" | ||
| edition = "2021" |
|
|
||
| # The Kani Rust Verifier for checking safety of the code | ||
| kani: | ||
| @command -v kani >/dev/null || \ |
There was a problem hiding this comment.
My comment was not to open the discussion about that, it's to keep the same approach eveywhere:
https://github.com/hypernetix/rclib/pull/2/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52R8
We have a makefile function to install cargo tools, let's reuse that function
| .arg(Arg::new("timeout").long("timeout").short('t').help("Request timeout in seconds (after connection)").default_value("300").num_args(1)) | ||
| .arg(Arg::new("openapi-file").long("openapi-file").help("Path to OpenAPI spec file").num_args(1)) | ||
| .arg(Arg::new("mapping-file").long("mapping-file").help("Path to mapping YAML file").num_args(1)) | ||
| .arg( |
There was a problem hiding this comment.
By looking at the clap documentation:
https://docs.rs/clap/latest/clap/
There are two ways of developing a CLI, builder and derive pattern. Both use macros, the first as declarative macro and the second as derived macro.
The derived macro is more used as you defined your Cli struct with everything that it's required and you save a lot of testings, as a lot of the code is generated automatically with that.
The builder as it's implemented right now it uses &str that there's no other way of checking of the existence of flags, but to have tests that check for that.
Also, the use of leak_str: https://github.com/hypernetix/rclib/pull/2/files/3e936a881b711332bc9614df73e316c82b3b0c9d#diff-390790450d579f92eb671a71adc4ee01b59d3088b94fe2ad62c611622a4181a9R18 is not idiomatic. It's doing unnecesary tricks.
3 participants
Current coverage is 82%