Re: New Package: sticky-shell

[Philip Kaludercic <philipk@posteo.net>]

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

Andrew De Angelis <bobodeangelis@gmail.com> writes:

> Hello everyone and thanks for all your work.
> I've made a new package that I would like to add to Elpa.

I assume you are referring to GNU ELPA (the only where contributors have
to sign the FSF CA?)

> The package provides a minor mode that creates a header in a shell buffer.
> The header shows the prompt above the top-visible line, allowing users to
> keep track of what prompt generated which output, even with very large
> multiple-line outputs.
> The package also has some customization features that can be used to
> determine which prompt to show in the header (it can be the latest-executed
> prompt, etc), and the look of the header.

Sounds interesting!

> This is the git repo: https://github.com/andyjda/sticky-shell, with
> additional info in the README and in the code's documentation.

Here are a few commends:


[-- Attachment #2: Type: text/plain, Size: 3411 bytes --]

diff --git a/sticky-shell.el b/sticky-shell.el
index 0fdc108..4d8aaa9 100644
--- a/sticky-shell.el
+++ b/sticky-shell.el
@@ -36,6 +36,8 @@
 
 ;;; Code:
 (eval-when-compile
+  ;; Why are these only required during compilation?  You appear to be
+  ;; using actual functions from these modules, not just macros.
   (require 'eshell)
   (require 'comint))
 
@@ -46,7 +48,6 @@
   "Display a sticky header with latest shell-prompt."
   :group 'terminals)
 
-
 (defcustom sticky-shell-get-prompt
   #'sticky-shell-prompt-above-visible
   "Function used by sticky-shell-mode to pick the prompt to show in the header.
@@ -55,21 +56,23 @@ Available values are: `sticky-shell-latest-prompt',
 `sticky-shell-prompt-above-cursor',
 `sticky-shell-prompt-before-cursor'
 or you can write your own function and assign it to this variable."
-  :group 'sticky-shell
-  :type 'function)
+  :type '(choice (function-item :tag "Prompt above visible" sticky-shell-prompt-above-visible)
+		 (function-item :tag "Latest prompt" sticky-shell-latest-prompt)
+		 (function-item :tag "Prompt above cursor" sticky-shell-prompt-above-cursor)
+		 (function-item :tag "Prompt before cursor" sticky-shell-prompt-before-cursor)
+		 other))
 
 (defcustom sticky-shell-prompt-modifiers
-  ()
+  '()
   "List of functions modifying the prompt before it is displayed in the header.
 See `sticky-shell-modified-prompt' for an explanation
 on how the functions are applied."
-  :group 'sticky-shell
   :type 'list)
 
 (defun sticky-shell-prompt-current-line ()
   "Return the current line and remove the trailing newline char."
   (let ((prompt (thing-at-point 'line)))
-    (aset prompt (- (length prompt) 1) 0) ; remove the newline ending char
+    (aset prompt (1- (length prompt)) 0) ; remove the newline ending char
     prompt))
 
 (defun sticky-shell-latest-prompt ()
@@ -78,6 +81,8 @@ on how the functions are applied."
   (save-excursion
     (goto-char (point-max))
     (forward-line -1)
+    ;; Perhaps you should pull this into a separate function, as the
+    ;; check appears quite often.  Another idea, as the pattern appease quite similar in general, you could also
     (if (derived-mode-p 'eshell-mode)
         (eshell-previous-prompt 1)
       (comint-previous-prompt 1))
@@ -129,23 +134,25 @@ macro-expands to:
  (upcase
   (funcall sticky-shell-get-prompt))
  \\='face \\='minibuffer-prompt)"
+  ;; The case distinction appears unnecessary (thread-first (foo)) is
+  ;; the same as (foo).
   (if sticky-shell-prompt-modifiers
       `(thread-first
          (funcall sticky-shell-get-prompt)
          ,@sticky-shell-prompt-modifiers)
+    ;; Perhaps it would be better/cleaner if
+    ;; `sticky-shell-prompt-modifiers' were a list of function that
+    ;; all get applied on the result of (funcall
+    ;; sticky-shell-get-prompt) in order?
     (funcall sticky-shell-get-prompt)))
 
 ;;;###autoload
 (define-minor-mode sticky-shell-mode
   "Minor mode to show the previous prompt as a sticky header."
-  :group 'comint
   :global nil
-  :lighter nil
-  (if sticky-shell-mode
-      (setq-local header-line-format
-                  (list '(:eval
-                          (sticky-shell-modified-prompt))))
-    (setq-local header-line-format nil)))
+  (setq-local header-line-format
+	      (and sticky-shell-mode
+		   '((:eval (sticky-shell-modified-prompt))))))
 
 (provide 'sticky-shell)
 ;;; sticky-shell.el ends here

[-- Attachment #3: Type: text/plain, Size: 369 bytes --]


> Let me know if you have any questions and if there are any issues with
> adding this to the Package Archive. (I do not have push-access to the git
> repository)

I don't think there should be any issue.  The only thing I would
recommend would be to consider adding an .elpaignore file to avoid
adding the screenshots to the tarball.

> Thank you!
>
> Best,
> Andrew