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

Show file tree
Hide file tree
Changes from all 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
4 changes: 4 additions & 0 deletions cmd2/v.v
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ fn main() {
execute: main_fn
}

// Coment
timbasel marked this conversation as resolved.
Show resolved Hide resolved
v_cmd.add_flag(Flag {
flag: .bool
name: 'verbose'
Expand All @@ -36,6 +37,7 @@ fn main() {
])
*/

// Comment
v_cmd.parse(os.args)
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 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

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

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.

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.

}

Expand All @@ -59,6 +61,7 @@ fn main_fn(cmd Command)? {
}

// try finding a tool that matches first argument
// Comment
if tool_exists(cmd.args[0]) {
Copy link
Author

Choose a reason for hiding this comment

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

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 ?

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.

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

mut args := ['exec']
args << cmd.args
Expand All @@ -68,6 +71,7 @@ fn main_fn(cmd Command)? {
return
}

// Comment
if os.exists(cmd.args[0]) || os.exists(cmd.args.last()) {
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 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 👍

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.

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

mut args := ['build']
args << cmd.args
Expand Down