Re: wrap-search 3.3.5

[Stefan Monnier]

Emanuel Berg [2022-08-25 20:50:26] wrote:
> Philip Kaludercic wrote:
>> Could you elaborate what you meant to say with this message.
>> Should it be added to ELPA?
> Sure!

For GNU ELPA, you'd first need to sign the relevant paperwork
(see further down).
For NonGNU ELPA, you'd need to put this in a public Git repository.
(I think for MELPA, the same would apply).

Some comments about your code:

> ;; `wrap-search' searches forward by default, but it wraps
> ;; and continues up and until the search start position if
> ;; necessary. This is so one doesn't have to wonder if the
> ;; target is north or south in the buffer.

You might want to mention `isearch-wrap-pause` and explain the advantage
of your package over what we get with (setq isearch-wrap-pause 'no).

> (defun wrap-string-data-p (str)
>   "Return STR if it is a string but not the empty string, \"\"."
>   (and (stringp str)
>        (not (string= str ""))
>        str) )

Since you package is small, I'd prefer it doesn't use up the whole
`wrap-` namespace, so I'd recommend you rename this function
`wrap-search-string-data-p`.

Also, in all the places where you use this function, you use the return
value as a boolean, so you could simplify it to

    (defun wrap-search-string-data-p (str)
      "Return non-nil if it is a string but not the empty string, \"\"."
      (and (stringp str)
           (not (string= str ""))) )

But in addition, I notice that it's only ever called with `prev-str` as
argument and that variable can never hold anything else than nil or
a non-empty string (because we test (string= "") before setting it).
So you can just remove the function altogether and just test
`prev-str` instead.  Also that var is global and it's never set back to
nil, so so the nil case should only occur once per Emacs session, and
I'd simplify the code by initializing the var not to nil but to some
dummy default like "unspecified search string" so you don't even need to
handle the nil case.

> (let ((prev-str)
>       (prev-case)
>       (prev-rev)
>       (prev-beg)
>       (prev-end) )
>   (defun wrap-search-again ()
>     (interactive)
>     (if (wrap-string-data-p prev-str)
>         (wrap-search prev-str prev-case prev-rev prev-beg prev-end)
>       (message "no search has been done") ))
>   (declare-function wrap-search-again nil)

Yeah, I'd welcome patches to `bytecomp.el` to handle such code better so
you don't need `declare-function`.

>   (defun show-prev-str ()
>     (if (wrap-string-data-p prev-str)
>         (let*((len   (length prev-str))
>               (max   10)
>               (short (when (< max len)
>                        (substring prev-str 0 max) )))
>           (format "[%s] " (if short
>                               (format "%s..." short)
>                             prev-str) ))
>       ""))

You can use `truncate-string-to-width` here.

>     (interactive
>      `(,(read-string (format "search: %s" (show-prev-str)))

The convention is to put the default before the `:` and wrapped in
"(default ...)" rather than "[...]".  Since Emacs-28 you can use
`format-prompt` to do that.

If (like me) you prefer the "[...]" format, you might like to use
`minibuffer-electric-default-mode` and (setq
minibuffer-eldef-shorten-default t).

>        ,(or (equal current-prefix-arg '( 4))
>             (equal current-prefix-arg '(64)) )

AKA
     ,(member current-prefix-arg '((4) (64)))

>        ,(or (equal current-prefix-arg '(16))
>             (equal current-prefix-arg '(64)) )

AKA
     ,(member current-prefix-arg '((16) (64)))

>       (let*((case-fold-search (not case))
>             (pos (point))
>             (data (if rev (list #'search-backward end beg)
>                     (list #'search-forward beg end) ))
>             (search-f (car data))
>             (search-beg (cadr data))
>             (search-end (caddr data)) )

You can also write this as

    (pcase-let* ((case-fold-search (not case))
                 (pos (point))
                 (`(,search-f ,search-beg ,search-end)
                  (if rev (list #'search-backward end beg)
                    (list #'search-forward beg end))))

>         (if (apply (list search-f str search-end t))

AKA

    (if (funcall search-f str search-end t)

which is not just shorter but also more efficient.

>           (if (apply (list search-f str (+ pos (if rev 0 (length str))) t))

Same here.

As for the paperwork, you'd want to fill the form below and send it as
instructed so the FSF can send you the relevant paperwork to sign.


        Stefan