How can I write this function better?

How can I write this function better?

[user]

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.

Thanks.

(defun luser-0x0 ()
  "Upload region or file to https://0x0.st"
  (interactive) 
  (let ((temporary-file (make-temp-file "0x0" nil
					(file-name-extension (or (buffer-file-name)
								 (concat (buffer-name) ".txt")) 
							     t)))
	(buffer (list (point-min) (point-max)))
	(region (list (region-beginning) (region-end))))
    (apply 'write-region (append (if (region-active-p) region
				   buffer)
				 (list temporary-file)))
    (set-process-filter (start-process "0x0" nil "curl"
				       "-F" (format "file=@%s" temporary-file)
				       "https://0x0.st")
			(lambda (process output)
			  (kill-new (message output))))))


This is a function to upload text to the https://0x0.st service. I use 0x0
because it's raw text, which means people can download it without opening or
switching to a browser.

Text is uploaded via the curl command: ‘curl -F'file=@foo.text' https://0x0.st’.

Since we use curl, a file must be created, which is done with (make-temp-file...).
The third argument to (make-temp-file ...) is a suffix for the file, our usage
of it will now be explained.

Uploading files without extensions to 0x0.st causes the service to append the
.bin suffix which could cause people to be nervous about downloading the
file. For this reason we generate an appropriate extension when possible, or
‘.txt’ when buffers aren't files. I'm explicit (verbose) with the creation of
‘.txt’ for the sake of clarity.

I want this function to be dwim-like[1]. When uploading text, you're either
selecting a specific piece or everything in the current buffer. When writing
our text to our temporary file, we'll use write-region even when there is no
region, since the usage of write-file forces us to visit the written file.

The usage of apply on a list of arguments is for simplicity, and once again,
clarity. The intent is nicely obvious with the variable names.

To use curl, we must start a process. If we separated (set-process-filter ...)
from (start-process ...), say creating a function, the process filter would
never run because curl would already be finished (our function is sequential).

With the process started inside (set-process-filter ...) we're able to apply
the filter when the process starts and before it finishes. Our filter function
is the easiest part of the code! It displays the output in the echo area and
kills it.

[1] https://en.wikipedia.org/wiki/DWIM

Re: How can I write this function better?

[Marcin Borkowski]

On 2017-03-13, at 20:49, 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.
>
> Thanks.

Quick comments off the top of my head (maybe completely wrong...)

1. No need to create one-use variables (there are probably various
schools on that...).

2. I think there is no need for `apply', either.

3. Instead of the low-level-ish set-process-filter and start-process,
why not use something much simpler like `call-process-shell-command'?

Hth,

-- 
Marcin Borkowski

Re: How can I write this function better?

[John Mastro]

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. 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

Re: How can I write this function better?

[Patrick]

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

Re: How can I write this function better?

[Thien-Thi Nguyen]

[-- Attachment #1: Type: text/plain, Size: 1412 bytes --]


() Patrick <pma@rdorte.org>
() Wed, 15 Mar 2017 10:55:29 +0100

   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).

Generally, if you have:

(let ((proc (start-process ...)))
  OTHER-CODE
  (set-process-sentinel proc FUNC))

FUNC will not be run (due to ‘proc’ not accepting output) unless
OTHER-CODE does (directly or indirectly) ‘sit-for’, ‘sleep-for’,
or ‘accept-process-output’, or waits for terminal input (e.g.,
w/ ‘read-string’).  See: (info "(elisp) Output from Processes")
and ‘git grep -n -H -e set-process-\\\(sentinel\\\|filter\\\)’
output (Emacs source) for theory and practice, respectively.

Assigning the process sentinel (or filter) need not even occur
in the same block.  Indeed, any "later" point in time is fine
(although there is such a thing as "too late to be useful" :-D).
I use this one-process-multiple-filter-set-"later" approach in
gnugo.el, for example.

-- 
Thien-Thi Nguyen -----------------------------------------------
 (defun responsep (query)
   (pcase (context query)
     (`(technical ,ml) (correctp ml))
     ...))                              748E A0E8 1CB8 A748 9BFA
--------------------------------------- 6CE4 6703 2224 4C80 7502


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]