implementation comparision

util.lisp.orig

;; original implementation
;; hunchentoot;utils.lisp
;;
(let ((counter 0))
  (declare (ignorable counter))
  (defun make-tmp-file-name (&optional (prefix "hunchentoot"))
    "Generates a unique name for a temporary file. This function is
called from the RFC2388 library when a file is uploaded."
    (let ((tmp-file-name
           #+:allegro
           (pathname (system:make-temp-file-name prefix *tmp-directory*))
           #-:allegro
           (loop for pathname = (make-pathname :name (format nil "~A-~A"
                                                             prefix (incf counter))
                                               :type nil
                                               :defaults *tmp-directory*)
                 unless (probe-file pathname)
                 return pathname)))
      (push tmp-file-name *tmp-files*)
      ;; maybe call hook for file uploads
      (when *file-upload-hook*
        (funcall *file-upload-hook* tmp-file-name))
      tmp-file-name)))

util.lisp.v1

;; users are responsible for making sure *file-upload-hook* is called in their supplied implementation in *upload-filename-generator*
;;

;; make-tmp-file-name implementation is the same as the original one

(defun make-upload-filename ()
  (let ((designator (cond
                     ;; the old behaviour.
                     ((null *upload-filename-generator*) 
                      t)
                     ;; the new behaviour.
                     ((or (symbolp *upload-filename-generator*)
                          (functionp *upload-filename-generator*))
                      *upload-filename-generator*))))
    designator))

util.lisp.v2

;; we wrap the supplied one with our own lambda to ensure that *file-upload-hook* is called properly.

;;; make-tmp-file-name implementation is the same as the original one

(defun make-upload-filename ()
  (let ((designator (cond
                     ;; the old behaviour.
                     ((null *upload-filename-generator*) 
                      t)
                     ;; the new behaviour.
                     ((or (symbolp *upload-filename-generator*)
                          (functionp *upload-filename-generator*))
                      (lambda (&key field-name file-name content-type :allow-other-keys t)
                        (let ((filename (funcall *upload-filename-generator*
                                                 :field-name field-name
                                                 :file-name file-name
                                                 :content-type content-type
                                                 :allow-other-keys t)))
                          (when *file-upload-hook*
                            (funcall *file-upload-hook* filename))
                          filename))))))
    designator))

There is nothing wrong with v2 in principle. Minor comments:

The name "designator" is not needed.
The cond should be an etypecase

(etypecase upload-filename-generator
(null ....)
((or symbol function) ...)

The lambda should have (&rest args &key file-name &allow-other-keys) as lambda list and invoke upload-filename-generator with (apply upload-filename-generator args)

Is there a reason to only include file-name and not the others (field-name and content-type)? or it is just for the example, and I should include them in the code? (I know that not writing them does not have effect on calling the function, but there presence help in the making the code clearer)

(defun make-upload-filename ()
  (etypecase *upload-filename-generator* 
    ;; the old behaviour.
    (null t)
    ;; the new behaviour.
    ((or symbol function) 
     (lambda (&rest args &key field-name file-name content-type &allow-other-keys)
       (let ((filename (apply *upload-filename-generator* args)))
         (when *file-upload-hook*
           (funcall *file-upload-hook* filename))
         filename)))))

On Mon, May 21, 2012 at 6:38 PM, Ala'a Mohammad ***@***.*** wrote: Is there a reason to only include file-name and not the others (field-name and content-type)? or it is just for the example, and I should include them in the code? (I know that not writing them does not have effect on calling the function, but there presence help in the making the code clearer)

It does not help making the code cleaner, and it also makes it harder to maintain. Giving things that are used only once a name is taxing the human reader (i.e. they have to remember names that are used only once). There is no advantage in destructuring the keyword argument, but it is cluttering the code and makes it harder to see what is really going on. I was even wrong when I suggested that the keyword argument filename should be passed down. This is because I overlooked the fact that there exists a file-name argument and a filename local variable. For maximum clarity, everything that is unimportant should not be written down to expose the real functionality as much as possible.

…

-Hans

Remove the &key arguments. The fact that upload-filename-generator accepts keyword arguments is completely irrelevant for this wrapper lambda. It wraps the invocation to invoke the hook function, and that is all that the code should express.

(defun make-upload-filename ()
  (etypecase *upload-filename-generator* 
    ;; the old behaviour.
    (null t)
    ;; the new behaviour.
    ((or symbol function) 
     (lambda (&rest args)
       (let ((filename (apply *upload-filename-generator* args)))
         (when *file-upload-hook*
           (funcall *file-upload-hook* filename))
         filename)))))

this should do it. and thanks for being patients with my questions. https://gist.github.com/2763054#gistcomment-326330

…

On Mon, May 21, 2012 at 8:48 PM, Hans Hübner ***@***.*** wrote: Remove the &key arguments.  The fact that _upload-filename-generator_ accepts keyword arguments is completely irrelevant for this wrapper lambda.  It wraps the invocation to invoke the hook function, and that is all that the code should express. --- Reply to this email directly or view it on GitHub: https://gist.github.com/2763054