Fix check option #79

0x61nas · 2023-07-12T10:52:53Z

Description

Add the missing code to override the check args in the config instance with the --check CLI option

Motivation and Context

Fix the issue that's introduced in refactor(config): refactor the config system #62

How Has This Been Tested?

Types of Changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation (no code change)
Refactor (refactoring production code)
Other

Checklist:

My code follows the code style of this project.
I have updated the documentation accordingly.
I have formatted the code with rustfmt.
I checked the lints with clippy.
I have added tests to cover my changes.
All new and existing tests passed.

0x61nas · 2023-07-12T12:40:23Z

I'm confused about how the args.check_args should split.

if the user passed --check="-h -v --help" we'll convert it into vec[vec["-h", "-v", "--help"]] which seems reasonable. but the check_args function well returns when any of the arguments are worked which is what we want in the config case because we know that every array should have related items (to test the same thing), but in this case, we don't so we should continue to test the other arguments in the same array even if one of them worked
if the user passed --check="-v" --check="-h,--help" we'll convert it into vec[vec["-v"], vec["-h", "--help"]] which seems reasonable also.but we'll have the same issue in the second array if the -h is succeeded, but In this case that what's the user most likely wants
and the last issue, if the user passed --check="-v" --check="-h,--help" --check="-V --version" we'll convert it into vec[vec["-v"], vec["-h", "--help"], vec["-V", "--version"]], and the third array it'll ignored by

for arg_variants in [
          (config.check_version).then(|| &args[0]),
          (config.check_help && args.len() >= 2).then(|| &args[1]),
      ]

so we have to make a special case if the user used the --check option?

orhun · 2023-07-12T13:39:37Z

Hmm, it seems like a quite a bit of a thought experiment. I don't think we will allow things like --check="-h -v --help" and only accept a singular argument as the --check parameter (such as --check "-h" --check "-v" --check "--help") so we can simply use vec[vec["-h"], vec["-v"], vec["help"]]?

so we have to make a special case if the user used the --check option?

Yup, if necessary.

0x61nas · 2023-07-12T14:07:10Z

I don't think we will allow things like --check="-h -v --help" and only accept a singular argument as the --check parameter (such as --check "-h" --check "-v" --check "--help") so we can simply use vec[vec["-h"], vec["-v"], vec["help"]]?

Then we'll still have the ignoring behavior issue + this would be a breaking change

Yup, if necessary.

I don't see any other solution at this moment.

in this case, I have two possible ways to solve this:

we can add a hidden boolean variable in the config struct and set it to true if the user uses the --check option and in the get_args_help we check on this and test all the arrays elements if it's true
we can simply set the first element in the config.check_args array to an empty array if the user passed the --check option and use this as our condition instead of the additional variable in the first solution.

I prefer the second way 'cause it simpler

Implement the second suggestion that's mentioned in <orhun#79 (comment)> and set the first element in the `config.check_args` array to an empty array if the user passed the --check option and use this as our condition in the `get_args_help` we check on this and test all the arrays elements

0x61nas · 2023-07-15T07:11:53Z

@orhun I can't figure out what makes test_check_help_args test fails. if you can provide some help for me I'll be grateful

orhun · 2023-07-15T09:09:53Z

src/helper/args/mod.rs

+        for arg_variants in if args[0].is_empty() {
+            [(args.len() > 1).then(|| &args[1..]), None]
+        } else {
+            [
+                (config.check_version).then(|| &args[..1]),
+                (config.check_help && args.len() >= 2).then(|| &args[1..]),
+            ]
+        }


The test_check_help_args test fail because this statement returns [None, None] which means no argument is checked.

orhun · 2023-07-15T09:10:36Z

src/helper/args/mod.rs

-            (config.check_help && args.len() >= 2).then(|| &args[1]),
-        ]
+        for arg_variants in if args[0].is_empty() {
+            [(args.len() > 1).then(|| &args[1..]), None]


I feel like this can be simplified. It is very hard to understand what this is doing at first glance 💫

orhun · 2023-07-15T09:11:14Z

src/helper/args/mod.rs

-    args: ArgsIter,
+    arg: &str,


What was the design choice behind removing the iterator and using a str here?

'cause I moved the iteration loop to the get_args_help so I can have more control over when to stop and when to continue

orhun · 2023-07-15T09:11:57Z

src/error.rs

+    /// Error that might if the argument is not found.
+    #[error("Argument not found.")]
+    ArgumentNotFoundError,


We didn't need this type of error so far, why do we have it now?

orhun · 2023-07-15T09:12:14Z

src/cli.rs

+            // Sets the first element to an empty vector to indicate that the all arguments should be checked.
+            args.push(Vec::new());


Hmm, is there a better way to handle this?

orhun · 2023-07-15T09:12:40Z

src/helper/args/mod.rs

+            if args[0].is_empty() {
+                // If the user uses the `--check` option then we check all the arguments.
+                arg_variants.iter().flatten().for_each(|arg| {
+                    let _ = check_arg(cmd, arg, verbose, output);


Why the result is ignored?

orhun · 2023-07-15T09:24:48Z

Btw, I'm still not entirely sure why do we need all of these changes. Isn't this the only thing that we need:

diff --git a/src/cli.rs b/src/cli.rs
index cd1ae29..d5a37c6 100644
--- a/src/cli.rs
+++ b/src/cli.rs
@@ -70,6 +70,9 @@ impl CliArgs {
     pub fn update_config(&self, config: &mut Config) {
         config.check_help = !self.no_help;
         config.check_version = !self.no_version;
+        if let Some(check_args) = &self.check_args {
+            config.check_args = Some(check_args.iter().map(|v| vec![v.to_string()]).collect());
+        }
         if let Some(CliCommands::Plz {
             ref man_cmd,
             ref cheat_sh_url,

Examples in README.md work just fine with this:

$ cargo run -- --check "\--silent" zps
(°ロ°)  checking 'zps --silent'
\(^ヮ^)/ success '--silent' argument found!

$ cargo run -- --check "help" --check "test" menyoki
(°ロ°)  checking 'menyoki help'
\(^ヮ^)/ success 'help' argument found!
---
---
(°ロ°)  checking 'menyoki test'
(×﹏×)      fail 'test' argument not found.

0x61nas · 2023-07-15T12:12:13Z

Examples in README.md work just fine with this:

hmmmmm, I guess I unintentionally overcomplicated the problem.

diff --git a/src/cli.rs b/src/cli.rs
index cd1ae29..d5a37c6 100644
--- a/src/cli.rs
+++ b/src/cli.rs
@@ -70,6 +70,9 @@ impl CliArgs {
     pub fn update_config(&self, config: &mut Config) {
         config.check_help = !self.no_help;
         config.check_version = !self.no_version;
+        if let Some(check_args) = &self.check_args {
+            config.check_args = Some(check_args.iter().map(|v| vec![v.to_string()]).collect());
+        }
         if let Some(CliCommands::Plz {
             ref man_cmd,
             ref cheat_sh_url,

if this simply works then let's close this PR and open another PR or if u want to commit it on the main branch

orhun · 2023-07-15T12:20:57Z

I think you can submit another PR 🐻 I haven't tested that approach thoroughly so feel free to add some tests if needed.

0x61nas · 2023-07-15T12:22:37Z

ok, i'll do that

0x61nas · 2023-07-15T12:44:18Z

if this simply works then let's close this PR and open another PR or if u want to commit it on the main branch

it didn't work with this example:

 $ cargo r -- --check='-H,help,--help' halp

    Finished dev [unoptimized + debuginfo] target(s) in 0.13s
     Running `/home/anas/.cargo-target/debug/halp --check=-H,help,--help halp`
(°ロ°)  checking 'halp -H,help,--help'
(×﹏×)      fail '-H,help,--help' argument not found.

orhun · 2023-07-15T14:26:56Z

--check='-H,help,--help' halp

That's now how it is supposed to be used though. It should be individual arguments such as --check '-H', --check 'help', etc.

0x61nas · 2023-07-16T05:34:16Z

but it works with the current version anyway and if we change that it might be a breaking change for someone

orhun · 2023-07-16T09:55:46Z

I don't think it is working.

$ halp --check "\-H,help,\-\-help" ls
(°ロ°)  checking 'ls -H,help,--help'
(×﹏×)      fail '-H,help,--help' argument not found.

Can you remove the default config file and try again?

0x61nas · 2023-07-16T13:09:42Z

Oh, I completely forget about the config file issue and I didn't notice that halp ignores my arguments

81: fix(cli): fix the check option r=orhun a=0x61nas  ## Description Add the missing code to override the check args in the config instance with the `--check` CLI option ## Motivation and Context - Fix the issue that's introduced in #62 ## How Has This Been Tested? - I trayed to pass `--check="-h"` and it worked as expected - I trayed to pass ` --check='-h' --check='-V'` and it worked as expected also ## Types of Changes - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Documentation (no code change) - [ ] Refactor (refactoring production code) - [ ] Other  ## Checklist: - [x] My code follows the code style of this project. - [x] I have updated the documentation accordingly. - [x] I have formatted the code with [rustfmt](https://github.com/rust-lang/rustfmt). - [x] I checked the lints with [clippy](https://github.com/rust-lang/rust-clippy). - [x] I have added tests to cover my changes. - [x] All new and existing tests passed. > This's an alternative to #79 Co-authored-by: Anas Elgarhy <anas.elgarhy.dev@gmail.com>

fix(cli): update the config.check_args based on --check option

cf6157a

0x61nas added 9 commits July 13, 2023 05:16

refactor(format): Run cargo fmt

6902d89

test(args): create tests for the check_arg method

b77d44f

test(args): update the check_version_args and check_help_args tests

423125f

test(args): fix the tests

c323a27

refactor(format): Run cargo fmt

64041fc

refactor(args): remove the unnecessary let binding

b87c5ff

test(args): fix the tests

d50926a

refactor(format): Run cargo fmt

9777257

orhun reviewed Jul 15, 2023

View reviewed changes

0x61nas closed this Jul 15, 2023

0x61nas reopened this Jul 15, 2023

0x61nas closed this Jul 15, 2023

0x61nas mentioned this pull request Jul 16, 2023

fix(cli): fix the check option #81

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix check option #79

Fix check option #79

0x61nas commented Jul 12, 2023 •

edited

Loading

0x61nas commented Jul 12, 2023

orhun commented Jul 12, 2023

0x61nas commented Jul 12, 2023

0x61nas commented Jul 15, 2023 •

edited

Loading

orhun Jul 15, 2023

orhun Jul 15, 2023

orhun Jul 15, 2023

0x61nas Jul 15, 2023

orhun Jul 15, 2023

orhun Jul 15, 2023

orhun Jul 15, 2023

orhun commented Jul 15, 2023

0x61nas commented Jul 15, 2023

orhun commented Jul 15, 2023

0x61nas commented Jul 15, 2023

0x61nas commented Jul 15, 2023

orhun commented Jul 15, 2023

0x61nas commented Jul 16, 2023

orhun commented Jul 16, 2023

0x61nas commented Jul 16, 2023

		// Sets the first element to an empty vector to indicate that the all arguments should be checked.
		args.push(Vec::new());

Fix check option #79

Fix check option #79

Conversation

0x61nas commented Jul 12, 2023 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of Changes

Checklist:

0x61nas commented Jul 12, 2023

orhun commented Jul 12, 2023

0x61nas commented Jul 12, 2023

0x61nas commented Jul 15, 2023 • edited Loading

orhun Jul 15, 2023

Choose a reason for hiding this comment

orhun Jul 15, 2023

Choose a reason for hiding this comment

orhun Jul 15, 2023

Choose a reason for hiding this comment

0x61nas Jul 15, 2023

Choose a reason for hiding this comment

orhun Jul 15, 2023

Choose a reason for hiding this comment

orhun Jul 15, 2023

Choose a reason for hiding this comment

orhun Jul 15, 2023

Choose a reason for hiding this comment

orhun commented Jul 15, 2023

0x61nas commented Jul 15, 2023

orhun commented Jul 15, 2023

0x61nas commented Jul 15, 2023

0x61nas commented Jul 15, 2023

orhun commented Jul 15, 2023

0x61nas commented Jul 16, 2023

orhun commented Jul 16, 2023

0x61nas commented Jul 16, 2023

0x61nas commented Jul 12, 2023 •

edited

Loading

0x61nas commented Jul 15, 2023 •

edited

Loading