Skip to content

Conversation

@dwinings
Copy link

I'm using your gem for an internal CLI project. I needed to add/change some features for the application, and wanted to share the features back upstream. In particular, catching every exception was causing us a lot of pain; catching every exception was forcing us to kill -9 every process/command that hung. I've modified the exception handling to provide a dedicated error for internal command resolution issues as well as catching Interrupt (Ctrl-C) while a command is executing, providing the expected behavior (Ctrl-C kills the current command while it's running, kills the CLI-Console shell if it isn't). Here is a relevant StackOverflow on this topic.

The default command feature I added is entirely optional and shouldn't affect existing usage of CLI-Console. Simply put, I am using it to expose a SQL-like interface, without having to write something like "query" before every command.

Thanks for you consideration.

Honestly, I don't know if catching any exceptions is correct behavior
here. It seems like most errors would want to be caller-handleable.
Also added an exception type to throw for commands that are not
recognized.
@davispuh
Copy link
Owner

Thanks :) Looks good, and yeah about Exception that wasn't good indeed.

Only I don't really like that default command implementation, especially a possible infinite loop due recursion in processCommand. I think it can be implemented better and more similar to how other commands works. Also would need a test for it.

Basically my idea is something like this

class Console
  def initialize(io)
    ...
    @DefaultCommand = method(:default)
  end

  attr_accessor :DefaultCommand

  def processCommand(commandString)
    ...
    else
      return @DefaultCommand.call(command, commandWords, commandString)
  end

  def default(command, commandWords, commandString)
     @IO.say("Command \"#{command}\" not recognized.")
     return -2
  end
  ...
end

# if you want to override it in application
console.DefaultCommand = apps_default_command

Basically move that "command not found" case in own function and it can be set to your own implementation. When passing all 3 params, there will be all required information for processing.

@davispuh
Copy link
Owner

What do you think? If you don't intend updating this I'll probably do it myself when I'll have time.

@dwinings
Copy link
Author

Hey, sorry I'm responding so slowly, been busy. I can probably refactor the default command stuff into something like that in the next couple days. Another issue I've been having has to do with how CLI-Console will strip quotes off of parameters in commands. Since the command I was writing wanted to be able to differentiate between string literals and non-strings, I needed CLI to not strip the strings. I wrote a new tokenizer that can handle both cases, but I'd need to add a switch into CLI to be able to choose which behavior. I might wrap that up at the same time I'm refactoring the default command implementation. Thanks for your feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants