option-derive-error output doesn't mention the option name that produced the error

[dan-passaro]

I'm working on a hobby project and I'm using clingon for option parsing. I added a custom option type for 2D coordinates so that I can specify an arbitrary spawn location when launching a simple game.

When I give no argument at all to the --spawn option, I get this nice error message:

$ sbcl --script run.lisp --spawn
No value specified for --spawn <INTEGER,INTEGER> option.
See 'program --help' for more details.

When I give an option that fails to parse, however, the output is less helpful:

$ sbcl --script run.lisp --spawn 3,4.1
whoops

In this instance it's partially my fault for giving an intentionally obtuse :reason to clingon:option-derive-error, but I think it would be nice if clingon mentioned which option it was trying to parse when the error occurred.

(I might provide a PR for this soon, so if you think this is a bad idea please let me know!)

[dnaeon]

Hey @dan-passaro ,

Could you please post some sample code that you have, so I can better understand what you are trying to accomplish?

Thanks!

[dan-passaro]

Sure, here's an example:

(defpackage :clingon-issue
  (:use :cl))
(in-package :clingon-issue)

(ql:quickload :clingon)
(ql:quickload :str)

(defclass 2d-coordinates (clingon:option)
  ()
  (:default-initargs :parameter "INTEGER,INTEGER")
  (:documentation "Parse an 'INTEGER,INTEGER' CLI option, producing a cons."))

(defmethod clingon:derive-option-value ((self 2d-coordinates) arg &key)
  (handler-case
      (let ((coords (mapcar 'parse-integer (str:split "," arg))))
        (destructuring-bind (x y) coords
          (cons x y)))
    (error () (error 'clingon:option-derive-error :reason "whoops"))))

(defmethod clingon:make-option ((kind (eql :2d-coordinates)) &rest rest)
  (apply 'make-instance '2d-coordinates rest))

(defun cli/options ()
  (list
   (clingon:make-option
    :2d-coordinates
    :long-name "spawn"
    :key :spawn
    :description "Set spawn point")))

(defun cli/handler (options)
  (format t "Spawn point: ~S~%" (clingon:getopt options :spawn)))

(defun cli/command ()
  (clingon:make-command
   :name "program"
   :options (cli/options)
   :handler 'cli/handler))

(defun main ()
  (clingon:run (cli/command)))

(main)

You can run using e.g. sbcl --load ~/quicklisp/setup.lisp --script clingon-issue.lisp

[dnaeon]

Hey @dan-passaro ,

An easy solution might be to provide more details about the error in the CLINGON:DERIVE-OPTION-VALUE method, e.g.

(defmethod clingon:option-usage-details ((kind (eql :option-info)) (option clingon:option) &key)
  (with-output-to-string (s)
    (cond
      ;; Short and long names are defined
      ((and (clingon:option-short-name option) (clingon:option-long-name option))
       (format s "-~A, --~A" (clingon:option-short-name option) (clingon:option-long-name option)))
      ;; We only have a short name defined
      ((clingon:option-short-name option)
       (format s "-~A" (clingon:option-short-name option)))
      ;; Long name defined only
      (t
       (format s "--~A" (clingon:option-long-name option))))))

(defmethod make-option-derive-error ((option clingon:option) arg)
  (let* ((info (clingon:option-usage-details :option-info option))
         (reason (format nil "Invalid value for option ~A: ~A" info arg)))
    (error 'clingon:option-derive-error :reason reason)))

(defmethod clingon:derive-option-value ((self 2d-coordinates) arg &key)
  (handler-case
      (let ((coords (mapcar 'parse-integer (str:split "," arg))))
        (destructuring-bind (x y) coords
          (cons x y)))
    (error () (make-option-derive-error self arg))))

This would now print the following.

Invalid value for option --spawn: 3,4.1

Another way, which we can fix this (and I think that's the better approach) is to add another slot to CLINGON:OPTION-DERIVE-ERROR condition, which carries an instance of the option which has failed.

• https://github.com/dnaeon/clingon/blob/master/src/conditions.lisp#L60

Then, this option instance can be inspected by the CLINGON:RUN method, and if we see such errors we can print the error there instead.

• https://github.com/dnaeon/clingon/blob/master/src/command.lisp#L751

This however would mean that we need to have a breaking API change, as then CLINGON:OPTION-DERIVE-ERROR will expect an instance of an option.

Let me know what do you think.

Thanks!