Re: [NonGNU ELPA] 11 new packages!

[Akib Azmain Turja <akib@disroot.org>]

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

Philip Kaludercic <philipk@posteo.net> writes:

> Akib Azmain Turja <akib@disroot.org> writes:
>
>> Philip Kaludercic <philipk@posteo.net> writes:
>>
>> [...]
>>
>>> I just realised that none of the patches can be applies, because the
>>> last commit in nongnu.git changed the package names from strings to
>>> symbols.  This makes applying the patches a nuisance.  Could you perhaps
>>> update the patches to use symbol for names, and make sure nothing has
>>> changed?
>>>
>>
>> Hmm, I think it's a good decision to use symbols.
>>
>> It's already midnight here, so I'll update the patches tomorrow.  
>
> Sure, no problem.  Thank you for updating the patches at all.
>
>>                                                                   BTW,
>> can you please you review my Eat package[1] (terminal emulator, you have
>> probably noticed it)?  I'm going to submit that too with the next batch
>> of patches.
>
> Looks interesting!  Here are my comments:
>
> diff --git a/eat.el b/eat.el
> index aa048a4..99b681e 100644
> --- a/eat.el
> +++ b/eat.el
> @@ -114,7 +114,7 @@ This is the default name used when running Eat."
>    :type 'boolean
>    :group 'eat-ui)
>  
> -(defcustom eat-term-scrollback-size 131072 ; 128 K
> +(defcustom eat-term-scrollback-size (* 128 1024) ; 128 K
>    "Size of scrollback area in characters.  nil means unlimited."
>    :type '(choice integer (const nil))
>    :group 'eat-term

Thanks.

> @@ -161,6 +161,7 @@ This is left disabled for security reasons."
>    "Custom type specification for Eat's cursor type variables.")
>  
>  (defcustom eat-default-cursor-type
> +  ;; Why are you checking the `default-value'?
>    `(,(default-value 'cursor-type) nil nil)
>    "Cursor to use in Eat buffer.
>  

Because the current buffer (which is it?) while loading might have
cursor-type locally set.

> @@ -198,6 +199,8 @@ blinking frequency of cursor."
>    :group 'eat-ui
>    :group 'eat-ehell)
>  
> +;; Do you think combining this and `eat-maximum-latency' into a single
> +;; variable makes sense?
>  (defcustom eat-minimum-latency 0.008
>    "Minimum display latency in seconds.
>  

I think it's better to keep them separate, so that I can describe better
in docstring.  And also I can't think of any good name of the combined
variable.

> @@ -445,6 +448,10 @@ If your process is choking on big inputs, try lowering the value."
>  
>  (put 'eat-term-color-bright-white 'face-alias 'eat-term-color-15)
>  
> +;; Perhaps you could automatically generate this block and make it
> +;; more maintainable?  `defface' is just a wrapper around
> +;; `custom-declare-face', so you could invoke that in a loop that
> +;; defines all the faces.
>  (defface eat-term-color-16
>    '((t :foreground "#000000" :background "#000000"))
>    "Face used to render text with 16th color of 256 color palette."

Yes, you are right.  I didn't know this at the time of writing that
piece of code.

> @@ -1732,6 +1739,9 @@ Treat LINE FEED (?\\n) as the line delimiter."
>                          (< (point) (point-max)))
>                      (< moved n))
>            (cl-incf moved)
> +          ;; I have a hunch that there should be a simpler way to do
> +          ;; this.  Something involving `line-end-position' and/or
> +          ;; `forward-line'.
>            (and (search-forward "\n" nil 'move)
>                 (= moved n)
>                 (goto-char (match-beginning 0))))

line-end-position doesn't work, it causes strange bugs that I can fix
only by put a (redisplay) before the call, something that I'm not going
to do at all for the sake of performance and reducing flickering.

> @@ -1775,7 +1785,7 @@ Treat LINE FEED (?\\n) as the line delimiter."
>    ;; Move to the beginning of line, record the point, and return that
>    ;; point and the distance of that point from current line in lines.
>    (save-excursion
> -    (let* ((moved (eat--t-goto-eol n)))
> +    (let ((moved (eat--t-goto-eol n)))
>        (cons (point) moved))))
>  
>  (defun eat--t-col-motion (n)

I think I do that very often.  TODO.  (A reminder for me.  I'll do any
change you suggested later, as I'm working to fix a bug in Eat.)

> @@ -1823,9 +1833,9 @@ Assume all characters occupy a single column."
>    (eat--t-col-motion n))
>  
>  (defun eat--t-repeated-insert (c n &optional face)
> -  "Insert C, N times.
> +  "Insert character C, N times.
>  
> -C is a character.  FACE is the face to use, or nil."
> +FACE is the face to use, or nil."
>    (let ((str (make-string n c)))
>      (insert (if face (propertize str 'face face) str))))
>  

Thanks, that's more clear.  TODO.

> @@ -1841,6 +1851,7 @@ where `*' indicates point."
>                  (point) 'eat--t-wrap-line nil limit)))
>    ;; Remove the newline.
>    (when (< (point) (or limit (point-max)))
> +    ;; What is the point of using `1value' here?
>      (1value (cl-assert (1value (= (1value (char-after)) ?\n))))
>      (delete-char 1)))
>  

To convince testcover.

> @@ -1855,7 +1866,7 @@ For example: when THRESHOLD is 3, \"*foobarbaz\" is converted to
>        ;; Go to the threshold column.
>        (eat--t-goto-col threshold)
>        ;; Are we at the end of line?
> -      (if (eq (char-after) ?\n)
> +      (if (eq (char-after) ?\n)         ;`eolp'?
>            ;; We are already at the end of line, so move to the next
>            ;; line and start from the beginning.
>            (forward-char)

After discovering the line-end-position bug, I'm afraid.

> @@ -1981,7 +1992,7 @@ Don't `set' it, bind it to a value with `let'.")
>  
>  (defun eat--t-reset ()
>    "Reset terminal."
> -  (let* ((disp (eat--t-term-display eat--t-term)))
> +  (let ((disp (eat--t-term-display eat--t-term)))
>      ;; Reset most of the things to their respective default values.
>      (setf (eat--t-term-parser-state eat--t-term) nil)
>      (setf (eat--t-disp-begin disp) (point-min-marker))

Oh, again...

> @@ -2011,7 +2022,7 @@ Don't `set' it, bind it to a value with `let'.")
>      (setf (eat--t-term-mouse-encoding eat--t-term) nil)
>      (setf (eat--t-term-focus-event-mode eat--t-term) nil)
>      ;; Clear everything.
> -    (delete-region (point-min) (point-max))
> +    (erase-buffer)
>      ;; Inform the UI about our new state.
>      (funcall (eat--t-term-grab-mouse-fn eat--t-term) eat--t-term nil)
>      (funcall (eat--t-term-set-focus-ev-mode-fn eat--t-term)

Obviously, no.  That'll remove narrowing, and narrowing to a
fundamentally part of Eat's design.

> @@ -2036,7 +2047,7 @@ display."
>      (unless (zerop n)
>        ;; Move to the Nth next column, use spaces to reach that column
>        ;; if needed.
> -      (eat--t-repeated-insert ?\  (- n (eat--t-col-motion n)))
> +      (eat--t-repeated-insert ?\s (- n (eat--t-col-motion n)))
>        (cl-incf (eat--t-cur-x cursor) n))))
>  

Didn't I push that?  Checking... no, my network connection didn't allow
me, and I forgot it afterwards.  Pushed.

>  (defun eat--t-cur-left (&optional n)
> @@ -2297,6 +2308,7 @@ of range, place cursor at the edge of display."
>    "Change character set to CHARSET.
>  
>  CHARSET should be one of `g0', `g1', `g2' and `g3'."
> +  (cl-assert (memq charset '(g0 g1 g2 g3)))
>    (setf (car (eat--t-term-charset eat--t-term)) charset))
>  

Thanks, that assertion should be there.  TODO.

>  (defun eat--t-write (str)
> @@ -2319,6 +2331,7 @@ CHARSET should be one of `g0', `g1', `g2' and `g3'."
>              ('dec-line-drawing
>               (let ((s (copy-sequence str)))
>                 (dotimes (i (length s))
> +                 ;; Perhaps you should pull out the alist into a `defconst'
>                   (let ((replacement (alist-get (aref s i)
>                                                 '((?+ . ?→)
>                                                   (?, . ?←)

Yes, I'm planning to use a hash-table (made with eval-when-compile).
TODO.

> @@ -2476,7 +2489,7 @@ N default to 1."
>                (eat--t-carriage-return)
>              ;; If we are somehow moved from the end of terminal,
>              ;; `eat--t-beg-of-next-line' is the best option.
> -            (if (/= (point) (point-max))
> +            (if (/= (point) (point-max)) ;(eobp)?
>                  (eat--t-beg-of-next-line 1)
>                ;; We are still at the end!  We can can simply insert a
>                ;; newline and update the cursor position.

Again, I'm afraid.

> @@ -2561,7 +2574,7 @@ N defaults to 0.  When N is 0, erase cursor to end of line.  When N is
>    (let* ((disp (eat--t-term-display eat--t-term))
>           (face (eat--t-term-face eat--t-term))
>           (cursor (eat--t-disp-cursor disp)))
> -    (pcase n
> +    (pcase-exhaustive n                 ;
>        ((or 0 'nil (pred (< 2)))
>         ;; Delete cursor position (inclusive) to end of line.
>         (delete-region (point) (car (eat--t-eol)))
> @@ -2619,7 +2632,7 @@ to (1, 1).  When N is 3, also erase the scrollback."
>    (let* ((disp (eat--t-term-display eat--t-term))
>           (face (eat--t-term-face eat--t-term))
>           (cursor (eat--t-disp-cursor disp)))
> -    (pcase n
> +    (pcase-exhaustive n
>        ((or 0 'nil (pred (< 3)))
>         ;; Delete from cursor position (inclusive) to end of terminal.
>         (delete-region (point) (point-max))

What's...  Checking docs...  Oh, thanks, that'll trigger an error in
case of no match.  TODO.

> @@ -2631,14 +2644,14 @@ to (1, 1).  When N is 3, also erase the scrollback."
>           ;; integer.
>           (let ((pos (point)))
>             ;; Fill current line.
> -           (eat--t-repeated-insert ?\  (1+ (- (eat--t-disp-width disp)
> +           (eat--t-repeated-insert ?\s (1+ (- (eat--t-disp-width disp)
>                                                (eat--t-cur-x cursor)))
>                                     (eat--t-face-face face))
>             ;; Fill the following lines.
>             (dotimes (_ (- (eat--t-disp-height disp)
>                            (eat--t-cur-y cursor)))
>               (insert ?\n)
> -             (eat--t-repeated-insert ?\  (eat--t-disp-width disp)
> +             (eat--t-repeated-insert ?\s (eat--t-disp-width disp)
>                                       (eat--t-face-face face)))
>             ;; Restore position.
>             (goto-char pos))))
> @@ -2656,7 +2669,7 @@ to (1, 1).  When N is 3, also erase the scrollback."
>           (if (not (eat--t-face-bg face))
>               (eat--t-repeated-insert ?\n (1- y))
>             (dotimes (_ (1- y))
> -             (eat--t-repeated-insert ?\  (eat--t-disp-width disp)
> +             (eat--t-repeated-insert ?\s (eat--t-disp-width disp)
>                                       (eat--t-face-face face))
>               (insert ?\n)))
>           ;; Fill the current line to keep the cursor unmoved.  Use

Pushed.

> @@ -2906,18 +2919,16 @@ position."
>      ;; on failure.
>      (when (and (<= scroll-begin (eat--t-cur-y cursor) scroll-end)
>                 (not (zerop n)))
> +      (eat--t-goto-bol)
> +      (eat--t-repeated-insert ?\  (1- (eat--t-cur-x cursor))
> +                              (and (eat--t-face-bg face)
> +                                   (eat--t-face-face face)))
>        (goto-char
> -       (prog1
> -           (progn
> -             ;; This function doesn't move the cursor, but pushes all
> -             ;; the line below and including current line.  So to keep
> -             ;; the cursor unmoved, go to the beginning of line and
> -             ;; insert enough spaces to not move the cursor.
> -             (eat--t-goto-bol)
> -             (eat--t-repeated-insert ?\  (1- (eat--t-cur-x cursor))
> -                                     (and (eat--t-face-bg face)
> -                                          (eat--t-face-face face)))
> -             (point))
> +       ;; This function doesn't move the cursor, but pushes all
> +       ;; the line below and including current line.  So to keep
> +       ;; the cursor unmoved, go to the beginning of line and
> +       ;; insert enough spaces to not move the cursor.
> +       (prog1 (point)
>           ;; Insert N lines.
>           (if (not (eat--t-face-bg face))
>               (eat--t-repeated-insert ?\n n)

I really wonder why, why I wrote the code like that?

> @@ -3189,7 +3200,8 @@ TOP defaults to 1 and BOTTOM defaults to the height of the display."
>                  nil t)))))
>      ;; Update face according to the attributes.
>      (setf (eat--t-face-face face)
> -          `(,@(when-let ((fg (or (if (eat--t-face-conceal face)
> +          ;; `while' is for side-effects, `and' is for expressions
> +          `(,@(and-let* ((fg (or (if (eat--t-face-conceal face)
>                                       (eat--t-face-bg face)
>                                     (eat--t-face-fg face))
>                                   (cond
> @@ -3201,31 +3213,31 @@ TOP defaults to 1 and BOTTOM defaults to the height of the display."
>                            :background
>                          :foreground)
>                        fg))
> -            ,@(when-let ((bg (or (eat--t-face-bg face)
> +            ,@(and-let* ((bg (or (eat--t-face-bg face)
>                                   (and (eat--t-face-inverse face)
>                                        (face-background 'default)))))
>                  (list (if (eat--t-face-inverse face)
>                            :foreground
>                          :background)
>                        bg))
> -            ,@(when-let ((underline (eat--t-face-underline face)))
> +            ,@(and-let* ((underline (eat--t-face-underline face)))
>                  (list
>                   :underline
>                   (list :color (eat--t-face-underline-color face)
>                         :style underline)))
> -            ,@(when-let ((crossed (eat--t-face-crossed face)))
> +            ,@(and-let* ((crossed (eat--t-face-crossed face)))
>                  ;; REVIEW: How about colors?  No terminal supports
>                  ;; crossed attribute with colors, so we'll need to be
>                  ;; creative to add the feature.
>                  `(:strike-through t))
>              :inherit
> -            (,@(when-let ((intensity (eat--t-face-intensity face)))
> +            (,@(and-let* ((intensity (eat--t-face-intensity face)))
>                   (list intensity))
> -             ,@(when-let ((italic (eat--t-face-italic face)))
> +             ,@(and-let* ((italic (eat--t-face-italic face)))
>                   (cl-assert (1value (eq (1value italic)
>                                          'eat-term-italic)))
>                   (list (1value italic)))
> -             ,@(when-let ((blink (eat--t-face-blink face)))
> +             ,@(and-let* ((blink (eat--t-face-blink face)))
>                   (list blink))
>               ,(eat--t-face-font face))))))
>  

Did you mean "when" instead of "while"?  TODO.

> @@ -3261,7 +3273,7 @@ MODE should be one of nil and `x10', `normal', `button-event',
>      (setf (eat--t-term-mouse-pressed eat--t-term) nil))
>    ;; Inform the UI.
>    (funcall (eat--t-term-grab-mouse-fn eat--t-term) eat--t-term
> -           (pcase mode
> +           (pcase-exhaustive mode
>               ('x10 :click)
>               ('normal :modifier-click)
>               ('button-event :drag)

TODO.

> @@ -3327,6 +3339,8 @@ output."
>    (funcall
>     (eat--t-term-input-fn eat--t-term) eat--t-term
>     (let ((rgb (color-values (face-foreground 'default))))
> +     ;; I wonder if it would make sense to write a `rx'-like macro for
> +     ;; generating these escape codes.
>       (format "\e]10;%04x/%04x/%04x\e\\"
>               (pop rgb) (pop rgb) (pop rgb)))))
>  

rx generates regexp that (sort of) parses string.  Do you the something
that does the opposite, i.e. joins parts into string?

But yes, there indeed a bit repetition.

> @@ -3788,17 +3802,15 @@ DATA is the selection data encoded in base64."
>                                        (rx ?\\))
>                                      output index)))
>             (if (not match)
> -               (progn
> -                 ;; Not found, store the text to process it later when
> -                 ;; we get the end of string.
> -                 (setf (eat--t-term-parser-state eat--t-term)
> -                       `(,state ,(concat buf (substring output
> -                                                        index))))
> -                 (setq index (length output)))
> +               ;; Not found, store the text to process it later when
> +               ;; we get the end of string.
> +               (setf (eat--t-term-parser-state eat--t-term)
> +                     `(,state ,(concat buf (substring output
> +                                                      index)))
> +                     index (length output))
>               ;; Matched!  Get the string from the output and previous
>               ;; runs.
> -             (let ((str (concat buf (substring output index
> -                                               match))))
> +             (let ((str (concat buf (substring output index match))))
>                 (setq index (match-end 0))
>                 ;; Is it really the end of string?
>                 (if (and (= (aref output match) ?\\)

Somehow I prefer to use one setq for each variable.  Is setting multiple
at once faster?  Benchmarking with "benchmark"...  Yes, about 1.5 times.
TODO.

> @@ -3937,7 +3949,7 @@ DATA is the selection data encoded in base64."
>          ;; Calculate the beginning position of display.
>          (goto-char (point-max))
>          ;; TODO: This part needs explanation.
> -        (let* ((disp-begin (car (eat--t-bol (- (1- height))))))
> +        (let ((disp-begin (car (eat--t-bol (- (1- height))))))
>            (when (< (eat--t-disp-begin disp) disp-begin)
>              (goto-char (max (- (eat--t-disp-begin disp) 1)
>                              (point-min)))

Again...

> @@ -3985,6 +3997,10 @@ DATA is the selection data encoded in base64."
>    "Setup the environment for TERMINAL and eval BODY in it."
>    (declare (indent 1))
>    `(let ((eat--t-term ,terminal))
> +     ;; This won't change much here, because the next line would
> +     ;; trigger a similar exception, but there might be some other
> +     ;; place where an explicit check could be of use.
> +     (cl-check-type eat--t-term eat--t-term)
>       (with-current-buffer (eat--t-term-buffer eat--t-term)
>         (save-excursion
>           (save-restriction

Hmm, it's a good idea on check early.

> @@ -4038,6 +4054,7 @@ To set it, use (`setf' (`eat-term-input-function' TERMINAL) FUNCTION),
>  where FUNCTION is the input function."
>    (eat--t-term-input-fn terminal))
>  
> +;; Perhaps require `gv' at the top?
>  (gv-define-setter eat-term-input-function (function terminal)
>    `(setf (eat--t-term-input-fn ,terminal) ,function))
>  

What do you mean by "top?"  At the top of the file?  What's the
advantage of that?

> @@ -4386,8 +4403,8 @@ client process may get confused."
>                      (setq tmp (get char 'ascii-character))
>                      (setq char tmp))))
>             (when (numberp char)
> -             (let* ((base (event-basic-type char))
> -                    (mods (event-modifiers char)))
> +             (let ((base (event-basic-type char))
> +                   (mods (event-modifiers char)))
>                 ;; Try to avoid event-convert-list if possible.
>                 (if (and (characterp char)
>                          (not (memq 'meta mods))

Again...

> @@ -4399,7 +4416,7 @@ client process may get confused."
>                   (let ((ch (pcase (event-convert-list
>                                     (append (remq 'meta mods)
>                                             (list base)))
> -                             (?\C-\  ?\C-@)
> +                             (?\C-\s  ?\C-@)
>                               (?\C-/ ?\C-?)
>                               (?\C-- ?\C-_)
>                               (c c))))

OK, now I must admit I missed this.  TODO.

> @@ -4608,9 +4625,9 @@ keywords:
>  EXCEPTIONS is a list of key sequences to not bind.  Don't use
>  \"M-...\" key sequences in EXCEPTIONS, use \"ESC ...\" instead."
>    (let ((map (make-sparse-keymap)))
> -    (cl-labels ((bind (key)
> -                  (unless (member key exceptions)
> -                    (define-key map key input-command))))
> +    (cl-flet ((bind (key)
> +                (unless (member key exceptions)
> +                  (define-key map key input-command))))
>        (when (memq :ascii categories)
>          ;; Bind ASCII and self-insertable characters except ESC and
>          ;; DEL.

Thanks.

> @@ -4618,7 +4635,7 @@ EXCEPTIONS is a list of key sequences to not bind.  Don't use
>          (cl-loop
>           for i from ?\C-@ to ?~
>           do (unless (= i meta-prefix-char)
> -              (bind `[,i])))
> +              (bind (vector i))))
>          ;; Bind `backspace', `delete', `deletechar', and all modified
>          ;; variants.
>          (dolist (key '( backspace C-backspace

Again a useless backquote usage...

> @@ -4780,11 +4797,12 @@ return \"eat-color\", otherwise return \"eat-mono\"."
>  (defvar eat--fast-blink-timer nil
>    "Timer for blinking rapidly blinking text.")
>  
> +(declare-function face-remap-add-relative "face-remap"
> +                    (face &rest specs))
> +(declare-function face-remap-remove-relative "face-remap" (cookie))
> +
>  (defun eat--flip-slow-blink-state ()
>    "Flip the state of slowly blinking text."
> -  (declare-function face-remap-add-relative "face-remap"
> -                    (face &rest specs))
> -  (declare-function face-remap-remove-relative "face-remap" (cookie))
>    (face-remap-remove-relative eat--slow-blink-remap)
>    (setq eat--slow-blink-remap
>          (face-remap-add-relative

TODO.

> @@ -4794,9 +4812,6 @@ return \"eat-color\", otherwise return \"eat-mono\"."
>  
>  (defun eat--flip-fast-blink-state ()
>    "Flip the state of rapidly blinking text."
> -  (declare-function face-remap-add-relative "face-remap"
> -                    (face &rest specs))
> -  (declare-function face-remap-remove-relative "face-remap" (cookie))
>    (face-remap-remove-relative eat--fast-blink-remap)
>    (setq eat--fast-blink-remap
>          (face-remap-add-relative
> @@ -4853,6 +4868,7 @@ return \"eat-color\", otherwise return \"eat-mono\"."
>      (face-remap-remove-relative eat--fast-blink-remap)
>      (remove-hook 'pre-command-hook #'eat--blink-stop-timers t)
>      (remove-hook 'post-command-hook #'eat--blink-start-timers t)
> +    ;; I think `mapc' comes in nicely here
>      (kill-local-variable 'eat--slow-blink-state)
>      (kill-local-variable 'eat--fast-blink-state)
>      (kill-local-variable 'eat--slow-blink-remap)

Yes.  TODO.

> @@ -5140,6 +5156,7 @@ ARG is passed to `yank-pop', which see."
>  ;; and commentary.
>  (defvar eat-mode-map
>    (let ((map (make-sparse-keymap)))
> +    ;; Why not use `kbd'?
>      (define-key map [?\C-c ?\M-d] #'eat-char-mode)
>      (define-key map [?\C-c ?\C-j] #'eat-semi-char-mode)
>      (define-key map [?\C-c ?\C-k] #'eat-kill-process)

I don't know, I somehow still prefer this format in Eat.

> @@ -5216,13 +5233,13 @@ ARG is passed to `yank-pop', which see."
>  (defun eat-semi-char-mode ()
>    "Switch to semi-char mode."
>    (interactive)
> -  (if (not eat--terminal)
> -      (error "Process not running")
> -    (setq buffer-read-only nil)
> -    (eat--char-mode -1)
> -    (eat--semi-char-mode +1)
> -    (eat--grab-mouse nil eat--mouse-grabbing-type)
> -    (force-mode-line-update)))
> +  (unless eat--terminal
> +    (error "Process not running"))
> +  (setq buffer-read-only nil)
> +  (eat--char-mode -1)
> +  (eat--semi-char-mode +1)
> +  (eat--grab-mouse nil eat--mouse-grabbing-type)
> +  (force-mode-line-update))
>  

Thanks.  TODO.

>  (defun eat-char-mode ()
>    "Switch to char mode."
> @@ -5302,7 +5319,7 @@ selection, or nil if none."
>  
>  (defun eat--bell (_)
>    "Ring the bell."
> -  (beep t))
> +  (ding t))
>  

Any difference?  Anyway, TODO.

>  \f
>  ;;;;; Major Mode.
> @@ -5340,6 +5357,7 @@ END if it's safe to do so."
>  (define-derived-mode eat-mode fundamental-mode "Eat"
>    "Major mode for Eat."
>    :group 'eat-ui
> +  ;; You can abbreviate parts of the definition with `setq-local'.
>    (make-local-variable 'buffer-read-only)
>    (make-local-variable 'buffer-undo-list)
>    (make-local-variable 'filter-buffer-substring-function)
> @@ -5372,8 +5390,8 @@ END if it's safe to do so."
>                   (:propertize
>                    "semi-char"
>                    help-echo
> -                  ,(concat "mouse-1: Switch to char mode, "
> -                           "mouse-3: Switch to emacs mode")
> +                  ,"mouse-1: Switch to char mode, \
> +mouse-3: Switch to emacs mode"
>                    mouse-face mode-line-highlight
>                    local-map
>                    (keymap

TODO.

> @@ -5469,7 +5487,7 @@ OS's."
>          (l (length string)))
>      (while (< i l)
>        (process-send-string process (substring string i (min j l)))
> -      (accept-process-output)
> +      (accept-process-output)           ;does this require a timeout?
>        (cl-incf i eat-input-chunk-size)
>        (cl-incf j eat-input-chunk-size))))
>  

Shamelessly stolen from Term mode.

> @@ -5910,6 +5928,7 @@ PROGRAM can be a shell command."
>              (eat-term-beginning eat--terminal)
>              (eat-term-end eat--terminal)
>            ;; TODO: Is `string-join' OK or should we use a loop?
> +          ;; ODOT: What should be the issue with `string-join'?
>            (eshell-output-filter
>             process (string-join (nreverse queue))))))))
>  

Ha ha, ODOT.  :)
Nothing, that's a reminder for me find out the most efficient code.
string-join creates garbage by throw all the string away, and using a
loop might cause Eshell to generate more garbage.

> @@ -5941,6 +5960,9 @@ PROGRAM can be a shell command."
>    (declare-function eshell-sentinel "esh-proc" (proc string))
>    (when (buffer-live-p (process-buffer process))
>      (with-current-buffer (process-buffer process)
> +      ;; As you use `cl-letf'/`cl-letf*' with `symbol-function' quite
> +      ;; frequently, perhaps it would make sense to define a macro
> +      ;; that makes the process less repetitive?
>        (cl-letf* ((process-send-string
>                    (symbol-function #'process-send-string))
>                   ((symbol-function #'process-send-string)

I was thinking that I would be nice to have a "advice-let" package.

> @@ -5974,6 +5996,8 @@ modify its argument to change the filter, the sentinel and invoke
>                                        (expand-file-name command))
>                                       args))))
>                (apply make-process plist)
> +            ;; `plist-put' is not destructive, you need to store the
> +            ;; return value.
>              (plist-put plist :filter #'eat--eshell-filter)
>              (plist-put plist :sentinel #'eat--eshell-sentinel)
>              (plist-put

Thanks, I'll use setf instead.  TODO.

> @@ -6328,7 +6352,7 @@ FN, `eat-exec', which see."
>              (push (cons var (symbol-value var)) variables)))
>          (with-current-buffer buf
>            (lisp-data-mode)
> -          (insert ";; -*- lisp-data -*-\n")
> +          (insert ";; -*- mode: lisp-data -*-\n") ;just a suggestion
>            (eat--trace-log time 'create 'eat width height
>                            variables))))))
>  

TODO.

> @@ -6495,7 +6519,7 @@ FN is the original definition of `eat--eshell-cleanup', which see."
>  (define-minor-mode eat-trace-mode
>    "Toggle tracing Eat terminal."
>    :global t
> -  :require 'eat
> +  :require 'eat                         ;why the `require', if the mode isn't autoloaded
>    :lighter " Eat-Trace"
>    (if eat-trace-mode
>        (progn

To shut package-lint up.

-- 
Akib Azmain Turja, GPG key: 70018CE5819F17A3BBA666AFE74F0EFA922AE7F5
Fediverse: akib@hostux.social
Codeberg: akib
emailselfdefense.fsf.org | "Nothing can be secure without encryption."

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