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.

Sign up for GitHub

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

Jump to bottom

TEMP Add comment #1

Closed
wants to merge 4 commits into from
Closed

TEMP Add comment #1

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
787fdce
TEMP Add comment
dedesite May 3, 2021
5231aab
Update exec.v
dedesite May 3, 2021
9e5b0b1
Update v.v
dedesite May 3, 2021
8074d80
Update v.v
dedesite May 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions cmd2/exec.v
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,11 @@ import v.pref
fn exec_cmd() &Command {
return &Command {
name: 'exec',
// Comment
usage: '<tool>'
Copy link
Author

Choose a reason for hiding this comment

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

What is the goal of usage, it will display command usage in help ?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, it is just for discribing the order of positonal arguments. e.g. <src> <dest>

Copy link
Owner

Choose a reason for hiding this comment

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

I am a bit conflicted about adding an explicit way of defining positional arguments. They can be useful in some cases but quickly make cli calls unreadable and hard to understand.

Copy link
Author

Choose a reason for hiding this comment

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

I think it could be really helpful to specify positional arguments, this way you can for example specify its type (eg. .path and cli will do an os.exists call) and document it.
Why would it make cli calls unreadable or hard to understand ?

Copy link
Owner

Choose a reason for hiding this comment

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

Lets say you have a function call that takes 3 numbers as arguments. The second one is immediately understandable without calling the help function, which is pretty useful when e.g. you used the arrow keys to go through your console history.

cmd 128 65 5

cmd --size 128 --offset 65 --value 5

Sure the first one would still be possible using the cli.args field, but for me it is about encouraging and discouraging behavior.

Copy link
Owner

Choose a reason for hiding this comment

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

Other than the I really like the validation idea. I can imagine there being a generalized solution, where you have predefined validation function for the most common use cases (e.g. path, email, etc.).

fn main() {
  mut cmd := Command{ name: 'root' }
  cmd.add_string(Flag{
    name: path
    validator: cli.validate_path
  })
  cmd.add_argument(Argument{
     name: 'email'
     validator: cli.validate_email
  })
}

Copy link
Owner

Choose a reason for hiding this comment

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

Just FYI somebody else added the pre_execute and post_execute functions. In my opinion these serve no real purpose and will definitely remove them sooner rather then later. Having only one place of execution makes things simpler and when you want something executed before everything else, then just put it in front.
The validator approach mentioned in the previous comment is a different story. It serves a clearly defined purpose.

Copy link
Author

Choose a reason for hiding this comment

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

I understand what you mean about positional argument, and click does not support documenting them either, but we seems to agreed that allow to give them a type and a validation is something we want for cli.
For example, cli often take a path as an argument. Validating that this path exists, and that you have writing rights on it etc. Can be really Helpull.

In your validation example, I would prefer having a flag type "path" or "email" than give an existing validator, but it's a detail. As I see it, validator are for custom validation process. Eg : you may want to add check on the email provider, check that the filename is in a specific format, if it's a zip, that it contains specific file (real use case).

Copy link
Author

Choose a reason for hiding this comment

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

Also agreed for pre_execute and post_execute

Copy link
Owner

@timbasel timbasel May 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

In your validation example, I would prefer having a flag type "path" or "email"

I'm not a fan of this, because fundamentally the type is still a string, it just how you interpret/validate it that makes it a path, email, etc.

In the my example cli.validate_path and cli.validate_email are just predefined validation functions that cover the often used cases. You could just as easily provide your own validator like this:

cmd.add_string(&Flag{
  name: 'email'
  validator: fn(value string) ?string {
     email := cli.validate_email(value)? // you could even reuse and modify existing ones
     if !email.ends_with('@gmail.com') {
        return err('only gmail allowed')
     }
     return email
  }
}

But thinking about it, a path type could definitely be useful, should we add autocompletion to the module. Because there we must know if a flag/arg should be a path or not.

description: 'Execute a tool defined in your project.'
execute: exec_fn,
// Comment
flags: [
Copy link
Author

Choose a reason for hiding this comment

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

I also like the declarative approach. But, it also add another "step" to retrieve the value.
This what https://github.com/alexflint/go-arg, click and other tool try to avoid.
In this way, I found the flag module simpler to use and it seems to be one of the main reason it is not removed in profit of cli.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, this definitely one of the major issues of the cli module.
The main problem is that you have to know all flags and commands prior to parsing, otherwise there are edge cases where it is impossible to know if an argument belongs to a flag or not.

Having add_flag return a pointer to the flag should alliviate some of this problem, but I'm still looking for a better solution.

Copy link
Author

Choose a reason for hiding this comment

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

I'm interested to know which are those edge cases. Does the flag module handle them or have flaws instead ?

Copy link
Owner

Choose a reason for hiding this comment

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

flag inherently does not have these edge cases because it does not handle (sub)commands.
The major edge case I came up with is the following:

// root --flag2 subcmd --flag1

fn main() {
  mut cmd := Command { name: 'root' }

  // when adding the first flag it is impossible to know if the 'subcmd'
  // is part of a later defined string flag, which would set the 'flag1' for 
  // the 'root' command, or an upcoming subcommand, because then it would belong 
  // to it (this is the case here) and be unset for the root cmd.
  cmd.get_bool(Flag{ name: 'flag1' }) 
  cmd.get_bool(Flag{ name: 'flag2' })

  mut subcmd := Command { name: 'subcmd' }

  subcmd.get_bool(Flag{ name: 'flag1' })

  // now knowing all the flag types makes it always possible to correctly 
  // match flags (and later arguments) to the correct command
  os.parse(os.args) 
}

Returning a pointer to the flag that has it's value set after the parse function is possible, but you can't return a 'real' value at that point due to the mentioned ambiguity.

Copy link
Author

Choose a reason for hiding this comment

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

In my proposition, parsing is sequential, so you wouldn't define flag1 on the first place, but do something like this :

// root --flag2 subcmd --flag1

fn main() {
  mut cmd := Command { name: 'root', args: os.args }

  // All the flags defined here belongs to the root command
  flag2 := cmd.get_bool(Flag{ name: 'flag2' })

  // As parsing is sequential, you shouln't add flag after the add_command
  cmd.add_command(Command { name: 'subcmd', execute: subcmd })
}

fn subcmd(cli cmd) {
     // All the flag here belong to "subcmd" command
     flag1 := cmd.get_bool(Flag{ name: 'flag1' })
}

Copy link
Owner

@timbasel timbasel May 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The Problem is that the value of flag1 (of cmd) depends on the type of the second flag. If flag2 is a boolean it would be 'false' because the --flag1 argument should count towards the subcmd. But now imagin flag2 being a string type, then 'subcmd' would be its value and therefore only the root command should be executed, which results in flag1 being true.

But I will think of a better example tomorrow, this one was the first thing that popped into my mind.

Copy link
Author

@dedesite dedesite May 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure I understood everything you wrote.
But if it the case, then I though about it and, finally, the add_command function needs to be called before flag declaration/getter in order to avoid mixing subcmd and flage string value.

Eg :

fn main() {
  mut cmd := Command { name: 'root', args: os.args }

  // cmd will know that there is a subcommand named "subcmd" and then stop the current parsing to it
  // eg : for "root --flag1 --flag2 subcmd something" it will only take into account ["--flag1", "--flag2"] in it's parsing
  cmd.add_command(Command { name: 'subcmd', execute: subcmd })

  // All the flags defined here belongs to the root command
  flag1 := cmd.get_bool(Flag{ name: 'flag1' })
  // flag2 can be a string and if there is something like
  // "root --flag1 -flag2 subcmd something
  // it will exit the program and display a usage error
  flag2 := cmd.get_string(Flag{ name: 'flag2' })
}

fn subcmd(cli cmd) {
     // All the flag here belong to "subcmd" command
     flag1 := cmd.get_bool(Flag{ name: 'flag1' })
}

Not sure if it reply to your question.
Edit : one problem remains : what if the flag2 is "subcmd" ? Don't know how other tools handle this case.

Copy link
Author

Choose a reason for hiding this comment

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

I'll maybe do a POC in order to test if what I have in mind is realistic or not.

Copy link
Author

Choose a reason for hiding this comment

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

You are right, it's not that easy.
It will need at least another function call.
I start thinking that the "sequential" approach has a lot of drawbacks also...
Will maybe continue digging this approach in other to have a realistic proposition, and then we'll have all in hands to choose which is the best API.
Maybe, we could then ask the community to take a vote.

Copy link
Owner

@timbasel timbasel May 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

From the usability point of view, your approach would be awesome, but you should never sacrifice correctness for it.

One hacky way we could achieve something similar, would be to return a reference to the value during flag creation. Internally these pointers would be stored as voidptr together with their type. In the parse function you could then cast them back to their original type and assign the flag value. The way V (should) handle refs could hide most of this from the end user, but currently V still some problems with it.

Flag {
timbasel marked this conversation as resolved.
Show resolved Hide resolved
flag: .string
Expand All @@ -23,6 +25,7 @@ fn exec_cmd() &Command {
}

fn exec_fn(cmd Command)? {
// Comment
is_verbose := cmd.flags.get_bool('verbose') or { false }
timbasel marked this conversation as resolved.
Show resolved Hide resolved
force_recompile := cmd.flags.get_bool('recompile') or { false }

Expand Down