TEMP Add comment

[dedesite]

In order to be able to annotate code

[dedesite]

First review

[dedesite]

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

[timbasel]

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

[timbasel]

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.

[dedesite]

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 ?

[timbasel]

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.

[timbasel]

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
  })
}

[timbasel]

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.

[dedesite]

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).

[dedesite]

Also agreed for pre_execute and post_execute

[timbasel]

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.

[dedesite]

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.

[timbasel]

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.

[dedesite]

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

[timbasel]

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.

[dedesite]

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' })
}

[timbasel]

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.

[dedesite]

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.

[dedesite]

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

[dedesite]

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.

[timbasel]

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.

[dedesite]

I think we can skip this step by providing os.args to the first Command initialisation, or maybe by creating a Program struct which will take the args. Parsing could be done "on the fly" after each new option, argument or command added, as proposed is this gist : https://gist.github.com/dedesite/1f1e26172729b022ef7833c934f8f0d8

[timbasel]

I have two reasons for having an explicit parse functions.
First of all it reduces the startup performance hit because you only have to do the 'expensive' parsing once. I know the impact is usually small, but it also happens in a very critical section of program execution for the user experience. So I would not want to take any chances, if not otherwise absolutely necessary.
Secondly it clearly separates the declaration from the execution. This of cause depends on what philosophy we want to follow. I think a declarative approach makes the structure of the cli inherently clearer and also better mimics how e.g. click is used with the decorators.

[dedesite]

I see your point. About performances I don't think this could make a noticeable difference. Even if the application handle dozens of options like youtube-dl, parse algorithm should be pretty simple and really fast to execute.
But you're right in a sense that it'll get calls more often.
About the distinction between declaration and execution, I see your point, but for me it adds complexity to the API and getting a flag value needs 4 step (cmd creation, declaration, parsing, getter) instead of 2 steps (cmd creation, getter).
As click or go-arg used decorator (or other specific comment for go, don't know go very well), it is not really comparable, because in the end, you end up with just on step : declaration which directly led to having variable set, either by function arguments (click) or via a instantiated struct (go-arg).

[timbasel]

Yes, finding a convenient way of defining and getting the value, that is possible in the current language without to much changes is pretty tricky.
The main goal should be to somehow combine the 'declaration' and the 'getter' steps, because this is where the most duplication and overhead is going to happen (see the parse_build_flags function in the cli2 prototype). 'cmd creation' and 'parsing' are pretty static in the overhead, so I believe there isn't really that much to gain.

[dedesite]

Yeah I don't see how declaration and getter steps can be combine with current V syntax.
I know there is some work going on in comptime conditons ($if) and metaprogramming, but not sure that it would someday allow the creation "on the fly" of a struct.
If we could initialized the struct fields with function that will be called on parsing, it could be great. But I don't think it is allowed (neither wanted) in V.

[timbasel]

Looking at the go-arg we don't need to create new struct types on the fly, just a way to add metadata to the fields.
The x/json2 module already uses $for to iterate over the fields of a struct, but only having the name of the field to define the options of a flag is too limiting.

[dedesite]

I'm not sure I understand what you are trying to do here ?
It some sort of default behavior without subcommand ?
In the case you do v some_v_file.v it'll call v build some_v_file.v right ?
For me, the API needs a way to define "arguments".
In my proposal, I'll do it this way :

to_build := get.get_string(cli.Arg{
		placeholder: 'filename'
		option_type: .file
	})

This way it can be display in help and some test could automatically be made (for example os.exists).
The get_command thing is clever as it allows to reuse subcommands if needed 👍

[timbasel]

Yes, this mimics the current default behavior of the V CLI.

[dedesite]

I don't understand this one. It's a new feature to add some v cli tool added by modules ? Like what npx is for node ?

[timbasel]

Yes, I wanted to see if it was possible to separate the tools that are needed for compiler development from the tools generally used. I think I came up with a pretty flexible solution, where you can define currently one but in the future possible many paths in the v.mod file and have the cli automatically build and execute the tool if the source is found in the specified directory

[dedesite]

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 ?

[dedesite]

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