Re: [ELPA] New package: colorful-mode

[Philip Kaludercic]

Elijah G <eg642616@gmail.com> writes:

> Hello, I want to submit the package colorful-mode to GNU ELPA.
>
> Colorful is a minor mode that allows preview of any color format such
> as color hex and color names in the current buffer in real time with a
> user-friendly approach.
>
> Colorful was based in rainbow-mode source, but colorful diverged in
> other way being more friendly, customizable and allow converting color
> to others with mouse clicks or keybindings such as css rgb/hsl to hex,
> or hex to color name and allow using prefix/suffix as indicator or
> highlight it using overlays, also it has support for mouse clicks for
> changing colors or using keybindings.
>
> The source can be found at: 
> https://github.com/DevelopmentCool2449/colorful-mode

Sounds nice.  Here are a few comments and suggestions:

diff --git a/colorful-mode.el b/colorful-mode.el
index d28db8d..82ddf5e 100644
--- a/colorful-mode.el
+++ b/colorful-mode.el
@@ -30,20 +30,18 @@
 ;;  friendly and effective way based on rainbow-mode.
 
 ;;; Code:
+
 
-;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
-;;                                  Libraries                                 ;
-;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+;;;; Libraries
 
 (require 'compat)
 
 (require 'color)
 (eval-when-compile (require 'subr-x))
+(eval-when-compile (require 'rx))
 
 
-;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
-;;                          Customizable User Options                         ;
-;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+;;;; User Options
 
 (defgroup colorful nil
   "Preview hex colors values in current buffer.."
@@ -279,9 +277,7 @@ mode is derived from `prog-mode'."
   :type '(choice boolean (const :tag "Only in prog-modes" only-prog)))
 
 
-;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
-;;                                   Keymaps                                  ;
-;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+;;;; Keymaps
 
 (defvar-keymap colorful-mode-map
   :doc "Keymap when `colorful-mode' is active."
@@ -290,19 +286,14 @@ mode is derived from `prog-mode'."
   "C-c c r" #'colorful-convert-and-change-color)
 
 
-;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
-;;                             Internal variables                             ;
-;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+;;;; Internal variables
 
 (defvar-local colorful-color-keywords nil
   "Font-lock colors keyword to highlight.")
 
 
-;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
-;;                             Internal Functions                             ;
-;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
-
-;;;;;;;;;; Base Convertion functions ;;;;;;;;;;
+;;;; Internal Functions
+;;;;; Base Convertion functions
 
 (defun colorful--percentage-to-absolute (percentage)
   "Convert PERCENTAGE to a absolute number.
@@ -315,12 +306,13 @@ If PERCENTAGE is above 100%, it's converted to 100."
       (string-to-number percentage))))
 
 (defun colorful--latex-rgb-to-hex (rgb)
-  "Return LaTex RGB as hexadecimal format.  RGB must be a string."
-  (let* ((rgb (string-split (string-remove-prefix "{rgb}{" rgb) ","))
-         (r (string-to-number (nth 0 rgb)))
-         (g (string-to-number (nth 1 rgb)))
-         (b (string-to-number (nth 2 rgb))))
-    (color-rgb-to-hex r g b)))
+  "Return LaTeX RGB as hexadecimal format.  RGB must be a string."
+  ;; just as an alternative idea:
+  (and (string-match 
"{rgb}{\\([[:digit:]]+\\),\\([[:digit:]]+\\),\\([[:digit:]]+\\)}" rgb)
+       (color-rgb-to-hex
+       (string-to-number (match-string 1 rgb))
+       (string-to-number (match-string 2 rgb))
+       (string-to-number (match-string 3 rgb)))))
 
 (defun colorful--latex-gray-to-hex (gray)
   "Return LaTex GRAY as hexadecimal format.  GRAY must be a string."
@@ -350,6 +342,8 @@ HSL must be a string."
                       (string-remove-prefix "hsl(" hsl)
                     (string-remove-prefix "hsla(" hsl))
                   (rx (one-or-more (any "," " " "\t" "\n""\r" "\v" "\f")))))
+           ;; what error is being ignored here?  if (nth n hsl) is
+           ;; nil, we can check this manually
             (h (ignore-errors (/ (string-to-number (nth 0 hsl)) 360.0)))
             (s (ignore-errors (/ (string-to-number (nth 1 hsl)) 100.0)))
             (l (ignore-errors (/ (string-to-number (nth 2 hsl)) 100.0)))
@@ -367,11 +361,11 @@ HSL must be a string."
   "Return color NAME as hex color format.
 DIGIT specifies which how much digits per component must have return value."
   (if-let* ((color-name (color-name-to-rgb name))
-            (color (append color-name `(,digit))))
+            (color (append color-name (list digit))))
       (apply #'color-rgb-to-hex color)
     (cdr (assoc-string name colorful-html-colors-alist))))
 
-;;;;;;;;;; User Interactive Functions ;;;;;;;;;;
+;;;;; User Interactive Functions
 
 ;;;###autoload
 (defun colorful-convert-and-change-color ()
@@ -419,12 +413,12 @@ DIGIT specifies which how much digits per component must 
have return value."
                     ("Convert and copy color." . copy)))
          (result (alist-get
                   (completing-read prompt choices nil t nil nil)
-                  choices nil nil 'equal)))
+                  choices nil nil #'equal)))
     (if (eq result 'copy)
         (colorful-convert-and-copy-color)
       (colorful-convert-and-change-color))))
 
-;;;;;;;;;; Coloring functions ;;;;;;;;;;
+;;;;; Coloring functions
 
 (defun colorful--change-color (ov &optional prompt color beg end)
   "Return COLOR as other color format.
@@ -582,9 +576,7 @@ converted to a Hex color."
     (colorful--colorize-match string beg end)))
 
 
-;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
-;;                         Extra coloring definitions                         ;
-;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+;;;; Extra coloring definitions
 
 (defvar colorful-hex-font-lock-keywords
   `((,(rx (seq (not (any "&"))
@@ -639,6 +631,7 @@ converted to a Hex color."
   "Function for add hex colors to `colorful-color-keywords'.
 This is intended to be used with `colorful-extra-color-keyword-functions'."
   (dolist (colors colorful-hex-font-lock-keywords)
+    ;; why are you using `add-to-list' here?
     (add-to-list 'colorful-color-keywords colors t)))
 
 (defvar colorful-color-name-font-lock-keywords
@@ -730,9 +723,7 @@ This is intended to be used with 
`colorful-extra-color-keyword-functions'."
     (add-to-list 'colorful-color-keywords colors t)))
 
 
-;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
-;;                           Minor mode defintinions                          ;
-;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+;;;; Minor mode defintinions
 
 (defun colorful--turn-on ()
   "Helper function for turn on `colorful-mode'."
@@ -759,7 +750,7 @@ This is intended to be used with 
`colorful-extra-color-keyword-functions'."
 ;;;###autoload
 (define-minor-mode colorful-mode
   "Preview any color in your buffer such as hex, color names, CSS rgb in real 
time."
-  :lighter nil :keymap colorful-mode-map
+  :global nil
   (if colorful-mode
       (colorful--turn-on)
     (colorful--turn-off))

Regarding the headers, my changes would make it compatible with
outline-minor-mode for Elisp, but if you prefer your style, then you can
also adjust `outline-regexp'.

-- 
        Philip Kaludercic on peregrine