Re: How can I write this function better?

[Patrick <pma@rdorte.org>]

On Tue, Mar 14 2017, John Mastro wrote:

> user <user0012@cocaine.ninja> wrote:
>> Hello help-gnu-emacs, I'm looking for pointers with my luser-0x0
>> function. I'm sure there are better ways I could write this. I'm not
>> asking to have you rewrite it for me, that'd be rude of me, but rather
>> what I should to look at, such as documentation or learning new
>> constructs etc. I'm _very_ new to programming and Lisp (learning Lisp
>> and Forth as my first languages; quite fun). An explanation, which I
>> first wrote for myself, is below the code.
>
> Here was my first thought on how I would personally write it. I wouldn't
> say it's better (they achieve the same thing) but perhaps it will give
> you an idea or two.
>
> The things I changed were:
>   - Make the region beginning and end positions arguments to the function
>     (which default to the region-or-buffer) when called interactively
>   - Only call `file-name-extension' when the buffer is visiting a file,
>     otherwise just use ".txt"
>   - Call `write-region' directly (rather than via `apply')
>   - Use `call-process' with a destination buffer rather than
>     `start-process' with a sentinel
>   - Delete the temporary file at the end unless an optional argument
>     says not to
>
> Regarding the last point, `start-process' does have an advantage, which
> is that the process is asynchronous.

This attempt takes into account most of these points (but I think that
start-process + sentinel is nice for these interactive use cases).  I'd
also be happy to hear about improvements.

(defun hacks/luser-0x0 (buffer &optional start end keep-temp-file)
  "Upload BUFFER contents to https://0x0.st, maybe only from START to END.

If called interactively, it switches to the result buffer when
the process has finished.  If called from elisp, it returns the
process."
  (interactive (cons (current-buffer)
		     (if (region-active-p)
			 (list (region-beginning) (region-end))
		       (list (point-min) (point-max)))))
  (let* ((target (url-generic-parse-url ;; "https://httpbin.org/post"
		  "https://0x0.st"))
	 (tmp-file-name (expand-file-name
			 (concat (make-temp-name "0x0-data")
				 (if (buffer-file-name)
				     (concat "." (or (file-name-extension (buffer-file-name)) "txt"))
				   ".txt") )
			 temporary-file-directory))
	 (proc (start-process
		"curl 0x0"
		(get-buffer-create (format "* curl 0x0 for %s (copied into %s) *"
					   (buffer-name buffer)
					   tmp-file-name))
		"curl"
		"-F"
		(format "file=@%s"
			(with-temp-file tmp-file-name
			  (insert-buffer-substring buffer start end)
			  tmp-file-name))
		(url-recreate-url target))))
    (process-put proc 'tmp-file tmp-file-name)
    (process-put proc 'keep-temp-file keep-temp-file)
    (if (called-interactively-p)
	(set-process-sentinel proc
			      (lambda (proc event)
				(cond
				 ((string= event "finished\n")
				  (unless (process-get proc 'keep-temp-file) (delete-file (process-get proc 'tmp-file)))
				  (switch-to-buffer (process-buffer proc))
				  (message "Process %s finished" (process-name proc)))
				 (t (with-current-buffer (process-buffer proc)
				      (insert event))))))
      proc)))


It should also allow simultaneous uploads.  Note how you can define the
process and then add properties or set a sentinel `later' in the
function, took me some time to figure this out.  But I still don't know
if that's always the case (feedback very welcome).

The calls to the `url-' functions are not necessary, but I think it's
good to know about them.

You might also want to look at this very useful tool:
https://github.com/pashky/restclient.el.

Hope this helps (and happy to hear about improvements),

> Regarding the last point, `start-process' does have an advantage, which
> is that the process is asynchronous. However, that may not matter for
> this, if the upload will proceed quickly and/or you would wait for it to
> finish before moving on anyway. For instance, if the upload takes a
> while, then it might be odd to have its output added to your kill-ring
> at some point later anyway.
>
> Here's the code (lightly tested):
>
>     (defun luser-0x0 (beg end &optional keep-file)
>       "Upload region from BEG to END https://0x0.st.
>     When called interactively, BEG and END default to the bounds of
>     the region if active, or the buffer otherwise. If KEEP-FILE is
>     non-nil, do not delete the temporary file that was uploaded."
>       (interactive
>        (if (use-region-p)
>            (list (region-beginning) (region-end))
>          (list (point-min) (point-max))))
>       (let ((file (make-temp-file "0x0" nil
>                                   (if (buffer-file-name)
>                                       (file-name-extension (buffer-file-name) t)
>                                     ".txt"))))
>         (write-region beg end file)
>         (with-temp-buffer
>           (call-process "curl" nil (current-buffer) nil
>                         "-F" (concat "file=@" file) "https://0x0.st")
>           (kill-new (message "%s" (buffer-string))))
>         (if keep-file
>             file
>           (delete-file file t)
>           nil)))
>
> Hope that helps
>
>         John


--
Patrick