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.

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

enhance function with argparse #5835

Open
danielb2 opened this issue Apr 20, 2019 · 9 comments
Open

enhance function with argparse #5835

danielb2 opened this issue Apr 20, 2019 · 9 comments
Labels

Comments

@danielb2
Copy link
Contributor

@danielb2 danielb2 commented Apr 20, 2019

fish, version 3.0.2

Currently function takes option -a NAMES (among) others to assign readable names to arguments.

It would be nice if an --flag option was also available which makes use of the built-in argparse command.

Example usage:

function recode -a input --flag "h/help" --flag "e/encoding" -d "use ffmpeg to convert a video file to another format"
  
  if [_$flag_h]
    fish_func_help()
  end
  
  set extension (string replace -i -r '^.*?(\w*)$' '$1' $input)
  set output (string replace -r '\.([^.]*)$' .$_flag_encoding $input).$extension
  
  ffmpeg -i "$input" -c:v $_flag_encoding -c:a copy -c:s copy "$output"
end

Since function is already aware of the name of the function and what options are available for argparse, it should be able to output a nice help message as well (current argparse error is a bit much)

@faho
Copy link
Member

@faho faho commented Apr 26, 2019

To be honest this just seems like a bit of a superfluous shortcut.

it should be able to output a nice help message as well (current argparse error is a bit much)

That's #3404 - argparse prints errors the same way all of our builtins do, and that was quite verbose. It's been shortened. With function t; argparse -n banana h/help v/version -- $argv; end; t -w you'd get:

banana: Unbekannte Option '-w'
~/.config/fish/functions/t.fish (Zeile 3): 
	argparse -n banana h/help v/version -- $argv;
	^
in function 't'
	called on standard input
	with parameter list '-w'

and now you'll get

banana: Unbekannte Option '-w'
~/.config/fish/functions/t.fish (Zeile 3): 
	argparse -n banana h/help v/version -- $argv;
	^
in function 't' with arguments '-w'

@faho faho added the RFC label Apr 26, 2019
@danielb2
Copy link
Contributor Author

@danielb2 danielb2 commented Apr 26, 2019

To be honest this just seems like a bit of a superfluous shortcut.

I'm not sure how it's superfluous anymore than parsing input arguments with -a NAMES is superfluous. Is C/C++ superfluous to ASM ? It's about being productive and ease of use.

@faho
Copy link
Member

@faho faho commented Apr 26, 2019

I'm not sure how it's superfluous anymore than parsing input arguments with -a NAMES is superfluous. Is C/C++ superfluous to ASM ? It's about being productive and ease of use.

This is just rote text substitution, for a feature that, on the micro-scale, is barely used (you do argparse maybe once per function).

At best, it is

function foo --argparse a/alpha b/beta

vs

function foo
    argparse -n foo a/alpha b/beta -- $argv

With the added point that:

  • The argument parsing in the "--argparse" example is weird - options usually don't take a list of arguments

  • It's hard to customize the behavior - e.g. do you want to return 1 if the option parsing fails? Do you want to use any of the other argparse options like --stop-at-nonopt? (Do we really want to add these to function, of all things?)

In essence, "there is only one way to do it" covers my main objection.


As an alternative, there are a few things we could do to make argparse easier to use:

  • Default to parsing $argv if no -- thing has been given.
  • Default to using the current function name in the error message (which would make -n usually unnecessary)

which would change the second example to:

function foo
    argparse a/alpha b/beta

@danielb2
Copy link
Contributor Author

@danielb2 danielb2 commented Apr 26, 2019

This is just rote text substitution, for a feature that, on the micro-scale, is barely used (you do argparse maybe once per function).

The difference here is once you starting using --flag (or --argparse) is that anything specified with --argument-names goes out the window which is unintuitive.

The argument parsing in the "--argparse" example is weird - options usually don't take a list of arguments

I don't follow this comment. Just to clarify, my example is a function that would take several flags. Like you may ls -lhr. ls here takes three flags (long format, suffix for space, reverse).

It's hard to customize the behavior - e.g. do you want to return 1 ...

That's a good question. function could easily grow large. Perhaps tackling the majority of use-cases would be the way to go.

As an alternative, there are a few things we could do to make argparse easier to use

it would be nice to add to the list that the original --argument-names should ignore flags parsed by argparse, but I'm not sure how that's doable.

@faho
Copy link
Member

@faho faho commented Apr 26, 2019

I don't follow this comment. Just to clarify, my example is a function that would take several flags.

Which makes it longer than the argparse solution once you have two or three flags, simply by having to specify --flag again.

it would be nice to add to the list that the original --argument-names should ignore flags parsed by argparse, but I'm not sure how that's doable.

I think it might be nicer to add positional argument support to argparse.

So you'd do

function foo
    argparse a/alpha b/beta --positional input

would add $input, if given outside of an argument option. But on the other hand, this would basically be set -l input $argv[1] after argparse.

@danielb2
Copy link
Contributor Author

@danielb2 danielb2 commented Apr 26, 2019

Which makes it longer than the argparse solution once you have two or three flags, simply by having to specify --flag again.

ah, it was just an initial suggestion. list is fine.

would add $input, if given outside of an argument option. But on the other hand, this would basically be set -l input $argv[1] after argparse.

That's exactly what --argument-names does now, and I think that's perfectly reasonable

@faho
Copy link
Member

@faho faho commented Apr 26, 2019

That's exactly what --argument-names does now, and I think that's perfectly reasonable

Not quite. --argument-names names the first X arguments before any parsing is done.

E.g. if you call foo --option argument input, it'll do the equivalent of set -l input --option. Then you do argparse, and it leaves just input in $argv, but $input is still set to --option.

Whereas if you did argparse o/option= --positional input -- $argv, $input would be input - equivalent do doing argparse ...; set -l input $argv[1].

@danielb2
Copy link
Contributor Author

@danielb2 danielb2 commented Apr 26, 2019

Not quite. --argument-names names the first X arguments before any parsing is done.

you're right. I got confused. yeah, that would be good.

@faho
Copy link
Member

@faho faho commented Apr 26, 2019

Default to parsing $argv if no -- thing has been given

Okay, one problem with that:

If you forget the --, we can't tell. If all of the arguments happen to look like optspecs, it even succeeds. If one doesn't, it looks like a broken optspec, and not a premature argument.

E.g.

argparse h/help hello
# which should be
argparse h/help -- hello

couldn't error argparse: Missing -- separator, but would have to error argparse: Invalid option spec 'hello' at char 'e'. Though the error could be improved, it would still be possible that it's a broken optspec.

faho added a commit that referenced this issue Apr 27, 2019
This makes the `--name` option usually unnecessary.

See #5835.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants