devnote-dev / cling

A modular, non-macro-based command line interface library

Hello 👋

I am a software developer from the UK working on various open–source and commercial projects. I am also a hobbyist photographer, athletics enthusiast and experimental videographer.

Languages

Stats

cling's Issues

Semantics of `post_run` hooks

The current flow of a command's execution runs through the pre_run hook -> run main method -> post_run hook. However, if pre_run or run are interrupted or stopped, post_run is never called.

cling/src/cling/executor.cr

Lines 61 to 66 in 0dcfb17

	begin
	resolved_command.run executed.parsed_arguments, executed.parsed_options
	resolved_command.post_run executed.parsed_arguments, executed.parsed_options
	rescue ex
	resolved_command.on_error ex
	end

This means that post_run is effectively limited to pre_run and run working successfully. So in cases where cleanup needs to be performed before an application exits, your only option is to handle it in pre_run, run or the on_error hook method. That leaves 2 possible options for post_run: to call this method after pre_run/run but not directly require them to exit successfully, or to remove it entirely.

Despite my large advocacy for Cling and its structure, I have never used post_run for any existing CLI applications. It was mostly inspired from Cobra's PostRun methods which operates on a similar premise: postRun is deferred and will always run regardless of the state of the PreRun and Run methods. Assuming none of the methods fail, PostRunE/PostRun is called just before execution ends.

Cling does not differentiate between erroneous and non-erroneous execution methods like Cobra does with "E" suffixed methods because exceptions can be raised at any point of the program making everything erroneous. That means post_run could effectively be changed to always be called as the final part of execution, with any exceptions from it or previous hook methods still being funneled to the on_error hook method.

While this sounds like a clear-cut solution, there are some undefined semantics as to what precedence post_run has in regards to exception handling (or rather, ignoring) which up until this point, has been preceded by the on_error hook method. Would this enforce a stricter exception handling structure? What affects would this impose on existing applications? And most importantly, is it worth it?

`on_invalid_option` hook is executed from the wrong command

Given the following code example, the on_invalid_option hook method is executed from Foo not Bar:

require "cling"

class Bar < Cling::Command
  def setup : Nil
    @name = "bar"

    add_option "baz", type: :single
  end

  def run(arguments : Cling::Arguments, options : Cling::Options) : Nil
  end

  def on_invalid_option(message : String)
    puts "Uh oh"
  end
end

class Foo < Cling::Command
  def setup : Nil
    @name = "foo"

    add_command Bar.new
  end

  def run(arguments : Cling::Arguments, options : Cling::Options) : Nil
  end
end

Foo.new.execute ARGV

$ crystal test.cr bar --baz
Unhandled exception: Missing required argument for option 'baz' (Cling::CommandError)
  from lib/cling/src/cling/command.cr:223:7 in 'on_invalid_option'
  from lib/cling/src/cling/executor.cr:48:7 in 'handle'
  from lib/cling/src/cling/command.cr:182:7 in 'execute'
  from test.cr:29:1 in '__crystal_main'
  from /usr/share/crystal/src/crystal/main.cr:115:5 in 'main_user_code'
  from /usr/share/crystal/src/crystal/main.cr:101:7 in 'main'
  from /usr/share/crystal/src/crystal/main.cr:127:3 in 'main'
  from /lib/x86_64-linux-gnu/libc.so.6 in '??'
  from /lib/x86_64-linux-gnu/libc.so.6 in '__libc_start_main'
  from /home/devonte/.cache/crystal/crystal-run-test.tmp in '_start'
  from ???

Removing "(required)" with options that have a default value specified

Example:

Options:
	-i, --input-path       Specify the path to the input file (required)
	-o, --output-path      Specify the path to the output directory (required) (default: out)

I think having "(required)" makes sense in the first case, but not the second one.

Alternatively, it would make sense if passing -o without a value would cause an error with the same message as when -i is missing:

Unhandled exception: Missing required argument for option 'i' (Cling::ExecutionError)

Exiting from `pre_run` hooks

For pre_run hook methods, there is the option to stop command execution to the main run method if the value returned is false. However, there are no checks in place to determine the actual exit code for this, the Executor simply returns which results in a 0 exit code:

cling/src/cling/executor.cr

Lines 52 to 54 in 279db9a

	begin
	res = resolved_command.pre_run executed.parsed_arguments, executed.parsed_options
	return unless res.nil? || res

This is obviously not ideal and even misleading for people designing applications that rely on the exit code for debugging or informational purposes. There are already ways to get around this, for example, raising a specific exception that gets funneled to the on_error hook method which then terminates the program with the appropriate exit code. But I think there should be a better way of doing this, even if it's just a shorthand method built into the Command class.