davetron5000 / gli

Make awesome command-line applications the easy way
http://davetron5000.github.io/gli
Apache License 2.0
1.26k stars 102 forks source link

./app command [--flag]... command [--flag]... <sub-command [--flag]...> : `too many arguments for command` #322

Closed notjames closed 1 month ago

notjames commented 1 month ago

From my reading such should be allowed but mine is not working:

require 'gli'

class Main
  extend GLI::App
    ...
    ...
    ...
  desc 'Main Command 1'
  command :auth do |auth|
    ...
    ...
    ...
    auth.action do |global_options, options, args|
    ...
    ...
    ...
      auth = Auth.new(email, password, global_options[:c])
      unless auth.login
        warn 'Error: Authentication failed'
      end
    ...
    ...
    ...
      puts 'Authenticated'
    end
  end

  desc 'Main Command 2'
  command :videos do |videos|
    ...
    ...
    ...
    puts '1'
    ...
    ... flags and switches
    ...

    puts '2'
    videos.desc 'Main Command 2 SubCommand 1'
    videos.command [:ls, :list] do |list|
      puts '3'
      list.action do |global_options, options, args|
          vl = Library::Videos.new
        vl.list
      end
    end

    puts '4'
    videos.desc 'Main Command 2 SubCommand 2'
    videos.command [:rm, :delete] do |rm|
    puts '5'
      rm.action do |global_options, options, args|
        del = Library::Videos.new
        del.delete
      end
    end

    puts '6'
    videos.desc 'Main Command 2 SubCommand 3'
    videos.command [:dl, :download] do |dl|
    puts '7'
      dl.action do |global_options, options, args|
        dl = Library::Videos.new
        dl.download
      end
    end
  end

  pre do |global_options, command, options, args|
    contents = self.read_config(global_options[:config])
    global_options.merge!(contents) unless contents.nil?
    true
  end

  def self.read_config(config_file)
    config = ConfigFile.new(config_file)
    config.load
  end

  def self.save_config(config_file, contents)
    config = ConfigFile.new(config_file)
    config.save(contents)
  end
end

If I request help for Main Command 1 then I get the properly displayed help and I also get the numbered debug output from Main Command 2 so it's obvious that parsing occurs throughout before getting called.

❯ bin/app help
1
2
3
4
5
6
7
NAME
...
...
...

BUT if I request help for Main Command 2, the help that's displayed is help from Main Command 1 (for both examples) and I also get the numbered output from Main Command 2. I also get the Too many arguments... error:

❯ bin/app --email some@mail.com auth --wallet wname videos help

1
2
3
4
5
6
7
error: Too many arguments for command

## help output displayed here ##

ls for Main Command 2:

❯ bin/app --email some@mail.com auth --wallet wname videos ls
1
2
3
4
5
6
7
error: Too many arguments for command

## help output displayed here ##

In summary, I can't seem to run a Main Commands 2 block in series with Main Command 1 because it causes Too many arguments for command exception:

Rationale behind what I'm trying to accomplish:

The tool has two main commands: auth and then videos with actions on videos for an API -

Currently the idea is that auth will always have to be performed and as such in sequence, the command to run against the API needs to be performed. Thus, two commands: auth and videos (in this case) The videos sub-command has actions. Currently, none of those actions are doing anything (seemingly not getting invoked), and of course the error occurring, Too many arguments for command which I suppose is a crux of my problem.

Future implementations of the script will read auth creds from the config file and only renew tokens if the expiration has occurred. Maybe the way to approach this dilemma is to run either auth OR videos (common practice) instead of both in series. But for now, this is my Moby Dick!! 🤣 So, any assistance would be appreciated.

Is this an issue or am I doing it all wrong?

Also, if I may piggy-back a question: suppose I do want to use the config file with cached creds and if they don't exist, I want the ability for the operator to run the auth argument (with switches). If the file exists, then I will want them to run the videos command -- a one-or-the-other situation (instead of running both in series).

I just tried to implement this using pre (and some refactoring by moving the blocks into their own respective classes) and it doesn't work at all:

class Main
  extend GLI::App

  wrap_help_text :verbatim
  subcommand_option_handling :normal
  arguments :strict

  accept(Date) do |string|
      Date.parse(string)
  end

  ArgsVideos.run(ARGV)

  pre do |global_options, command, options, args|
    begin
      contents = self.read_config(global_options[:config])
      global_options.merge!(contents) unless contents.nil?
    rescue JSON::ParserError => e
      warn format('Error: Unable to parse config file: %s', e.message)
      warn 'You will have to re-authenticate'
      ArgsAuth.run(ARGV)
    end
    true
  end

  def self.read_config(config_file)
    config = ConfigFile.new(config_file)
    config.load
  end

  def self.save_config(config_file, contents)
    config = ConfigFile.new(config_file)
    config.save(contents)
  end
end

All this does is always run ArgsVideos. The pre block never even gets touched.

davetron5000 commented 1 month ago

OK, you've got a lot going on here :)

First thing that might be confusing is that my_command videos help will not get help on the videos subcommand. That is interpreted as running the videos subcommand with the argument "help". To get help on that subcommand you must do my_command help videos or my_command help videos ls.

I created a version of your code from the top of the issue, removed the ... and it generally seemed to work and interpret the commands properly.

I'm not 100% following what you want to do, but I think you want something like:

You can do this with the pre block. I'm going to assume you need an email and password to authenticate and that authenticating returns some sort of token used to make further API calls. Hopefully whatever you are doing is close enough that this makes sense.

The idea is that you look at the command being run inside pre. If it's auth then you will save the credentials to the config file if they work. If the command is anything else, you auth from the config file or command line.

desc 'credentials file'
flag :credentials, default_value: File.join(ENV["HOME"],".app.credentials.json")

pre do |global,command,options,args|
  save_creds_to_file = command.name.to_s == "auth"
  credentials = if File.exist?(global[:credentials])
             JSON.parse(File.read(global[:credentials]))
           else
             {}
           end
  credentials = credentials.merge(global) # let --email override credentials file, e.g.

  begin
    api_token = Auth.authenticate!(credentials[:email], credentials[:password])
    if save_creds_to_file
      File.open(global[:credentials],"w") do |file|
        file.puts credentials.slice(:email,:password).to_json
      end
    end
  rescue => ex
    # Assume an exception of auth fails
    exit_now!("Authentication Failed: #{ex.message}")
  end
end

So, this would allow:

> rm ~/.app.credentials.json # no config
> app auth
# Fails, since email and pass not set
> app --email=valid@example.com --password=correct_password videos ls
# Works, but does not save the credentials
> app --email=valid@example.com --password=correct_password auth
# Works, saves the credentials
> app videos ls
# Works, since credentials are in the file

GLI has the concept of a config file that you could use, but it may not work exactly how you like.

Let me know if this helps.

notjames commented 1 month ago

Great. Really appreciate the assist. It helps, but we're not out of the woods. I refactored my code a bit modeled off what you provided only because I already had everything coded up and working except for the problems outlined in this issue. I think the auth part might be working-ish now. I still can't run this:

app --email thing@blah.com auth --wallet mywalletname videos ls

but I can run:

app --email thing@blah.com --wallet mywalletname 

and then run

app videos ls

without it failing the auth part. Let's assume we got that working. Where it fails now is just the videos command to work correctly:

require 'gli'

class Main
  extend GLI::App

  wrap_help_text :verbatim

  program_desc 'API CLI Tool'

  subcommand_option_handling :normal
  arguments :strict

  accept(Date) do |string|
    Date.parse(string)
  end

  desc 'Verbosity level'
  flag [:verbose], type: :boolean

  desc 'config file'
  flag [:c, :config],  type: String,
                   default_value: File.join(Dir.home, '.config', 'mypath', 'config.json')

  long_desc %{long description}

  desc 'Email address used to auth'
  flag [:email],   type: String

  desc 'Manage credential management to the api'
  command :auth do |auth|
    auth.desc 'Name of the KDE wallet to use'
    auth.long_desc %{If --wallet is used, --folder and --entry are required.
                     Use of --wallet will invoke the KDE Wallet to retrieve
                     the password.}
    auth.flag [:wallet], type: String

    auth.desc 'Name of the folder in the KDE wallet'
    auth.long_desc %{If --wallet is used, --folder and --entry are required.}
    auth.flag [:folder], type: String, default_value: 'application'

    auth.desc 'Name of the entry in the KDE wallet'
    auth.long_desc %{If --wallet is used, --folder and --entry are required.}
    auth.flag [:entry],  type: String, default_value: '< --email parameter >'

    auth.desc 'Authenticate with the api'
    auth.long_desc %{use either (KDE) --wallet or --email and --password. If --wallet
                     is used, --folder and --entry are required.}

    auth.action do |global_options, options, args|
      email    = global_options[:email]
      password = ENV['API_PASSWORD']

      options[:entry] = nil if options[:entry] =~ /--email/

      wallet   = options[:wallet]
      folder   = options[:folder]
      entry    = options[:entry] || email

      if wallet
        if folder.nil? || entry.nil?
          warn 'Error: --wallet requires --folder and --entry'
        end

        wallet = KWallet.new(wallet, folder, entry)

        wallet.get_password
        password = wallet.password

        if password.nil?
          warn 'Error: Unable to retrieve password from KDE Wallet'
        end
      end

      if password.nil?
        print 'Please enter your password: '
        password = $stdin.noecho(&:gets).chomp
        puts
      end

      auth = Auth.new(email, password, global_options[:c])
      unless auth.login
        warn 'Error: Authentication failed'
      end
      self.save_config(global_options[:c], {config:
                                             {wallet: options[:wallet],
                                              folder: folder,
                                              entry: entry}})
      puts 'Authenticated'
    end
  end

  desc 'Manage videos on the API'
  command :videos do |videos|
    videos.desc 'Manipulate videos on the API'

    videos.flag [:a, :all],               type: :boolean, default: true
    videos.flag [:t, 'by-title'],         type: :string
    videos.flag [:e, 'by-episode'],       type: :string
    videos.flag [:d, 'by-download-date'], type: :string
    videos.flag ['by-series'],            type: :string
    videos.flag ['by-season'],            type: :string

    videos.desc 'Actions on recordings on the API'

    videos.command :ls do |list|
      list.desc 'List videos on the API'
      list.action do |global_options, options, args|
        # Assuming Library::Videos.new.list is implemented
        vl = Library::Videos.new
        vl.list
      end
    end

    videos.desc 'Actions on recordings on the API'
    videos.command [:rm, :delete] do |rm|
      rm.desc 'Delete videos on the API'
      rm.action do |global_options, options, args|
        # Assuming Library::Videos.new.delete is implemented
        del = Library::Videos.new
        del.delete
      end
    end

    videos.desc 'Actions on recordings on the API'
    videos.command [:dl, :download] do |dl|
      dl.desc 'Download videos from the API'
      dl.action do |global_options, options, args|
        # Assuming Library::Videos.new.download is implemented
        dl = Library::Videos.new
        dl.download
      end
    end
  end

  pre do |global, command, options, args|
    begin
      thing = command.name.to_s == 'auth'

      if thing
        contents = self.read_config(global[:config])
        global.merge!(contents) unless contents.nil?
      end
    rescue JSON::ParserError => e
      warn format('Error: Unable to parse config file: %s', e.message)
      #ArgsAuth.run(ARGV)
      exit_now!('You will have to re-authenticate')
    end
    true
  end

  def self.read_config(config_file)
    config = ConfigFile.new(config_file)
    config.load
  end

  def self.save_config(config_file, contents)
    config = ConfigFile.new(config_file)
    config.save(contents)
  end
end

That's the args.rb with some sanitization. Once we get this whole thing figured out, I'll sanitize it more, but I think it helps if you see what I've got so far. Unfortunately, while the documentation is good, I think what I needed seemed like it was doable with GLI but it's not working as I'd hoped or expected. If you see anything glaringly wrong there, please let me know.

(Note: I know the auth code isn't perfect yet as it's barely managing happy path. I'll work on that later because I'm currently time constrained.)

Here's what happens now after the refactor and successfully running the auth bit alone:

✦ at 14:43:53 via  v3.1.4 ❯ bin/app videos ls
List videos
Delete videos
Download videos
List videos

I was expecting List videos only since ls was given.

Also, if I run one of your suggestions:

app --email ... auth --wallet ... videos ls

I get the error error: Too many arguments for command

davetron5000 commented 1 month ago

One thing you are trying to do that won't work is app --email … auth --wallet … videos ls because that is probably going to be interpreted as running the auth command with the --wallet flag and the arguments of "videos" and "ls". Since you are setting strict arguments, this is likely the source of the "too many arguments" error you are getting. The commands don't chain the way you are wanting them too. GLI will try to parse the command line and figure out what command you are trying to run, then run that command with whatever arguments are remaining. It only ever runs one command at a time.

I'm not following your second to last example bin/app videos ls. In particular I don't see how the code example you posted could generate any output like that, so not sure what you are expecting vs. what is happening. The use of desc almost may not be what you would expect, either.

For example:

desc "this is the description of the 'videos' command"
command :videos do |c|
  c.desc "this describes nothing - it's ignored"
  c.action do |g,o,a|
  end
end

desc "this is the description of the 'widgets' command"
command :widgets do |c|
  c.desc "this describes the flag --all"
  c.flag :all

  c.action do |g,o,a|
  end
end

One thing I find helpful when creating a complex multi-level UI is to get everything set up so the help is output the way I like, I then add in the logic. This way, you are sure that the UI you are creating is invokable the way you want without getting wrapped up in your logic.

Another thing that may help is to set GLI_DEBUG=true when invoking your command. That might print out more of what's happening internally.

notjames commented 1 month ago

So what you said, auth command with --wallet is the behavior I am going for. I'm allowing two different auth methods (determined by the switches): 1) basic auth with username and password (input from env or on TTY) or 2) extracted from system wallet. However, the last part in your sentence was "...and the arguments of videos and ls"; and what I'm going for there is that videos is a command of its own and it's sub commands (as written in the code) are interpreted for videos namely ls, rm, dl, etc.

but I think you answered my original question, though, which was would this be expected to work?:

bin/app --email <email> auth --wallet <walletname> videos --switches ls
        ^^^^^^^^^^^^^^^ global switch              |      |          |
                        |    |                     |      |          |
                        ^^^^ command 1             |      |          |
                             |                     |      |          |
                             ^^^^^^^^^^^^^^^^^^^^ command 1 switches |
                                                   |      |          |
                                                   ^^^^^ command 2   |
                                                          |          |
                                                          ^^^^^^^^^^ command 2 switches (which will work on actions of sub-commands of command 2)
                                                                     |
                                                                     ^^^^ sub command of command 2

...and if I'm interpreting your answer correctly, no (although now I'm wondering because if the commands get run sequentially then that IS what I was going for).

GLI will try to parse the command line and figure out what command you are trying to run, then run that command with whatever arguments are remaining. It only ever runs one command at a time.

This was my interpretation as well and what I was trying to for, which was: auth first and then run videos (after successful authing) with sub commands and actions (unless you mean that you can only have one main command. That part is not clear to me yet.) And that was and is not working. The auth always worked. The videos never worked, and I've tried several permutations of code trying to get it to work as desired.

I'm not following your second to last example

I was trying to point out that after I run bin/app auth (on its own) and then run bin/app videos ls (currently, as-is in the code block above) then that (the second to last block) is what is the output, which doesn't jive with what I'm trying to accomplish and I don't think it jives with my code so I was thinking there's a bug or I'm just wrong.

so...

✦ at 14:43:53 via  v3.1.4 ❯ bin/app videos ls
List videos
Delete videos
Download videos
List videos

is what's happening. What I expect is:

✦ at 14:43:53 via  v3.1.4 ❯ bin/app videos ls
List videos

or let's say I want to delete:

✦ at 14:43:53 via  v3.1.4 ❯ bin/app videos rm
Delete videos

or even (order will matter here so I'd have to code logic in, maybe, to make sure it doesn't remove first then try to list and download):

✦ at 14:43:53 via  v3.1.4 ❯ bin/app videos --all ls dl rm
List videos
Download videos
Delete videos

etc etc. so:

I don't see how the code example you posted could generate any output that...

me too!! Which was the main purpose, sorta, for this issue (among all the other things we've discussed). What I expect to happen is not happening.

notjames commented 1 month ago

Arrrgh! OK, I figured out my problem. It had nothing to do with GLI. It was the way I was initializing the Library::Videos class! I apologize!

However, you did answer my question, which I thank you.

So, may I ask one last one?

Is there a way to obtain global_options, options, flags, switches, etc outside commands or actions?

Here's an example why I'm seeking to do this:

1    auth.desc 'Name of the entry in the KDE wallet'
2    auth.long_desc %{If --wallet is used, --folder and --entry are required.}
3    auth.flag [:entry],  type: String, default_value: '< --email parameter >'

on line 3 (which is in my first command (auth) block), what I'd prefer to use as default_value: is the actual email address that's either provided by --email (global switch) or the email that's stored inside the config instead of "< --email parameter >". I haven't been able to successfully do this, but since there's seemingly no variable reference open to obtaining it, like there is in the command and action blocks through arguments, I can't seem to use it.

davetron5000 commented 1 month ago

You can access the global/options/flags from a pre block, which also passes the command.

If you were to use GLI's config system, I think that might do what you want, but that config system allows any flag or switch to be stored in the config and requires using the initconfig command on your app to bootstrap the config. So I don't think that's the UI you want.

You may need to hand-jam the order of precedence between options on your own.

You could have pre populate an internal $config object that has the value from the external file, overridden from the global switch. Then in the command block of auth you could allow a command/subcommand-level flag to override that

email = options[:email] || $config[:email]

At the end of the day, even though this is a wacky DSL, inside c.action |…| do is Ruby so you can always create normal classes/objects and call them from those action blocks if you find you have a lot of duplicated code.

notjames commented 1 month ago

Thank you for all your assistance... The DSL is a bit whacky in some places but once you get a hang of it, it's good.