changeset 5244:04811a268716

Be clearer in our error messages, #'canonicalize-inst-pair, #'canonicalize-spec lisp/ChangeLog addition: 2010-08-15 Aidan Kehoe <kehoea@parhasard.net> * specifier.el (canonicalize-inst-pair, canonicalize-spec): If a specifier tag set is correct, but an instantiator is not in an accepted format, don't error with the message "Invalid specifier tag set". Also, when we error, use error-symbols, for better structured error handling and more ease when testing. tests/ChangeLog addition: 2010-08-15 Aidan Kehoe <kehoea@parhasard.net> * automated/lisp-tests.el: (not, not, invalid-argument, invalid-argument): Check that error messages from the image specifier instantiator code are clearer than they used to be.
author Aidan Kehoe <kehoea@parhasard.net>
date Sun, 15 Aug 2010 15:42:45 +0100
parents 808131ba4a57
children 0d71bcf96ffd 02d875ebd1ea
files lisp/ChangeLog lisp/specifier.el tests/ChangeLog tests/automated/lisp-tests.el
diffstat 4 files changed, 62 insertions(+), 9 deletions(-) [+]
line wrap: on
line diff
--- a/lisp/ChangeLog	Sun Aug 15 13:29:10 2010 +0100
+++ b/lisp/ChangeLog	Sun Aug 15 15:42:45 2010 +0100
@@ -1,3 +1,12 @@
+2010-08-15  Aidan Kehoe  <kehoea@parhasard.net>
+
+	* specifier.el (canonicalize-inst-pair, canonicalize-spec):
+	If a specifier tag set is correct, but an instantiator is not in
+	an accepted format, don't error with the message "Invalid
+	specifier tag set".
+	Also, when we error, use error-symbols, for better structured
+	error handling and more ease when testing.
+
 2010-07-24  Aidan Kehoe  <kehoea@parhasard.net>
 
 	* cl-extra.el (concatenate):
--- a/lisp/specifier.el	Sun Aug 15 13:29:10 2010 +0100
+++ b/lisp/specifier.el	Sun Aug 15 15:42:45 2010 +0100
@@ -105,20 +105,23 @@
 	   ;; this will signal an appropriate error.
 	   (check-valid-instantiator inst-pair specifier-type)))
 
-	((and (valid-specifier-tag-p (car inst-pair))
-	      (valid-instantiator-p (cdr inst-pair) specifier-type))
+	((not (valid-instantiator-p (cdr inst-pair) specifier-type))
+	 (if noerror
+	     t
+	   (check-valid-instantiator (cdr inst-pair) specifier-type)))
+
+	((valid-specifier-tag-p (car inst-pair))
 	 ;; case (b)
 	 (cons (list (car inst-pair)) (cdr inst-pair)))
 
-	((and (valid-specifier-tag-set-p (car inst-pair))
-	      (valid-instantiator-p (cdr inst-pair) specifier-type))
+	((valid-specifier-tag-set-p (car inst-pair))
 	 ;; case (c)
 	 inst-pair)
 	 
 	(t
 	 (if noerror t
-	   (signal 'error (list "Invalid specifier tag set"
-				(car inst-pair)))))))
+	   (error 'invalid-argument "Invalid specifier tag set"
+		  (car inst-pair))))))
 
 (defun canonicalize-inst-list (inst-list specifier-type &optional noerror)
   "Canonicalize the given INST-LIST (a list of inst-pairs).
@@ -199,9 +202,14 @@
 
 	(if (not (valid-specifier-locale-p (car spec)))
 	    ;; invalid locale.
-	    (if noerror t
-	      (signal 'error (list "Invalid specifier locale" (car spec))))
-
+	    (if noerror
+		t
+	      (if (consp (car spec))
+		  ;; If it's a cons, they're probably not passing a locale
+		  (error 'invalid-argument
+			 "Not a valid instantiator list" spec)
+		(error 'invalid-argument
+		       "Invalid specifier locale" (car spec))))
 	  ;; case (b)
 	  (let ((result (canonicalize-inst-list (cdr spec) specifier-type
 						noerror)))
--- a/tests/ChangeLog	Sun Aug 15 13:29:10 2010 +0100
+++ b/tests/ChangeLog	Sun Aug 15 15:42:45 2010 +0100
@@ -1,3 +1,10 @@
+2010-08-15  Aidan Kehoe  <kehoea@parhasard.net>
+
+	* automated/lisp-tests.el:
+	(not, not, invalid-argument, invalid-argument):
+	Check that error messages from the image specifier instantiator
+	code are clearer than they used to be.
+
 2010-08-15  Aidan Kehoe  <kehoea@parhasard.net>
 
 	* automated/lisp-tests.el:
--- a/tests/automated/lisp-tests.el	Sun Aug 15 13:29:10 2010 +0100
+++ b/tests/automated/lisp-tests.el	Sun Aug 15 15:42:45 2010 +0100
@@ -2374,6 +2374,35 @@
 				     (garbage-collect))))))
  "checking we can amputate lists without crashing #'reduce")
 
+(Assert (not (eq t (canonicalize-inst-list
+		    `(((mswindows) . [string :data ,(make-string 20 0)])
+		      ((tty) . [string :data " "])) 'image t)))
+	"checking mswindows is always available as a specifier tag")
+
+(Assert (not (eq t (canonicalize-inst-list
+		    `(((mswindows) . [nothing])
+		      ((tty) . [string :data " "]))
+		    'image t)))
+	"checking the correct syntax for a nothing image specifier works")
+
+(Check-Error-Message invalid-argument "^Invalid specifier tag set"
+		     (canonicalize-inst-list
+		      `(((,(gensym)) . [nothing])
+			((tty) . [string :data " "]))
+		      'image))
+
+(Check-Error-Message invalid-argument "^Unrecognized keyword"
+		     (canonicalize-inst-list
+		      `(((mswindows) . [nothing :data "hi there"])
+			((tty) . [string :data " "])) 'image))
+
+;; If we combine both the specifier inst list problems, we get the
+;; unrecognized keyword error first, not the invalid specifier tag set
+;; error. This is a little unintuitive; the specifier tag set thing is
+;; processed first, and would seem to be more important. But anyone writing
+;; code needs to solve both problems, it's reasonable to ask them to do it
+;; in series rather than in parallel.
+
 (when (featurep 'ratio)
   (Assert (not (eql '1/2 (read (prin1-to-string (intern "1/2")))))
 	  "checking symbols with ratio-like names are printed distinctly")