-
Notifications
You must be signed in to change notification settings - Fork 6
Change the argument handling code to be more flexible. #10
base: master
Are you sure you want to change the base?
Conversation
r? @andreastt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this makes sense, but I would prefer better documentation of check_arg
.
src/runner.rs
Outdated
/// arguments, respecting the various platform-specific conventions. | ||
pub fn is_profile_arg(arg: &str) -> bool { | ||
/// contains an argument of a given names, respecting | ||
/// various platform-specific conventions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t think “given string” is very good documentation. I suggest something like this, provided names
is renamed matches
:
Check that a platform-specific argument string
arg
or arbitrary length (for example/Profile C:\\some\\path
or-p test -something -else
) contains one of the flags given in the array ofmatches
, which should be given without leading dashes, e.g.&["p", "profile"]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That isn't really right. It assumes that you have an array of arguments and are passing each arg from the array down into the function. It won't tokenize an argument string into an array of arguments.
I also think that matches
is a weird name, because it sounds like the result of a regex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That isn't really right. It assumes that you have an array of arguments and are passing each arg from the array down into the function.
Yes, so the caller will typically do e.g. args.iter().any(|x| check_arg(x, &["profile", "p"])) { … }
to check if each argument, x
, matches either profile
or p
in the platform-specific ways of the current system.
It won't tokenize an argument string into an array of arguments.
That’s not what I meant either. The first argument is the input as-is, the second argument is an array of platform-agonstic matches. If that gives you associations to regular expressions, I don’t think ‘names’ is any better.
The bottom line is that the documentation isn’t helpful in explaining what the expected input and output is. From the way it is written now, I could imagine using it like this:
check_args("-profile /foo", &["-profile", "--profile", "/profile"]);
The intention of the function is obviously not to do a ["-profile", "--profile", "/profile"].includes("profile")
check.
Instead of just exposing a function that checks for hardcoded arguments, expose a function that can check against a passed list of arguments.
d9173c1
to
8dd731b
Compare
Instead of just exposing a function that checks for hardcoded
arguments, expose a function that can check against a passed list of
arguments.