Re: [NonGNU Elpa] New package: ETT

[Philip Kaludercic]

John Task <q01@disroot.org> writes:

> [Disclaimer: A similar message was posted here a couple months ago by
>  me.  Sorry if that's an annoyance.  This one is clearer, though.]
>
> Hello.  My name is John Task and I'd like to submit a package to
> NonGNU ELPA.  I'm the author of this program and also (at the time of
> writing) the only contributor to the code.  As far as I can tell,
> everything is in line with the guidances and I plan to keep it that
> way.
>
> The URL of the repository is: https://gitlab.com/q01_code/ett
>
> The description is as follows:
>
> ----------------------------------------------------------------------
> Emacs Time Tracker (or short ETT) is a simple yet powerful time
> tracker for Emacs. Even though it's based on a minimalist plain
> text file, it can show statistics for current day, week, month or
> year, and even compare tags recording for the same item given any
> of these periods.
>
> Advanced features include percentages, graphs and icons.
>
> Clock-in with M-x ett-add-track, go to file with M-x ett-find-file,
> and get report with M-x ett-report. You probably want to bind these
> functions to easy keys.
> ----------------------------------------------------------------------
>
> The corresponding patch would be:
>
> ---
> From 72c7ba2c1ecd375be52b071d25217088e66e9abb Mon Sep 17 00:00:00 2001
> From: John Task <q01@disroot.org>
> Date: Tue, 2 May 2023 17:40:44 -0300
> Subject: [PATCH] New package
>
> ---
>  elpa-packages | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/elpa-packages b/elpa-packages
> index c333cc8bb3..385a76264d 100644
> --- a/elpa-packages
> +++ b/elpa-packages
> @@ -154,6 +154,11 @@
>    :readme "README.md"
>    :ignored-files ("doc/demo.gif"))
>  
> + (ett                :url "https://gitlab.com/q01_code/ett";
> +  :readme ignore
> +  :doc "ett-manual.org"
> +  :ignored-files ("ett-manual.org" "COPYING" "ETT.png" "doclicense.texi"))
> + 
>   (evil                       :url "https://github.com/emacs-evil/evil";
>    :ignored-files ("COPYING" "lib" "scripts")
>    :doc "doc/build/texinfo/evil.texi")

I would track the ignored files in a .elpaignore file that would look
something like this

--8<---------------cut here---------------start------------->8---
/ett-pkg.el
/ett-autoloads.el
/ett-manual.org
/COPYING
/ETT.png
/doclicense.texi
--8<---------------cut here---------------end--------------->8---

and I have prepared a few comments here:

diff --git a/ett.el b/ett.el
index c37d7f15b0..de6a017248 100644
--- a/ett.el
+++ b/ett.el
@@ -89,7 +89,7 @@ Something like '-->' or '====>' would be acceptable."
 Unlike `ett-separator', you can safely change this to anything you want."
   :type '(string))
 
-(defcustom ett-file (concat user-emacs-directory "tracks.ett")
+(defcustom ett-file (locate-user-emacs-file "tracks.ett")
   "File for storing ETT tracks."
   :type '(file))
 
@@ -125,10 +125,10 @@ size to be displayed.
 Only even numbers are allowed.  Indeed, it's a weird bug."
   :type '(natnum))
 
-(defcustom ett-graph-icon "│"
+(defcustom ett-graph-icon ?│
   "Icon used for building ETT graphs.
 It must be a single char string."
-  :type '(string))
+  :type 'character)
 
 (defface ett-heading
   '((((class color) (min-colors 88) (background light))
@@ -170,7 +170,7 @@ It must be a single char string."
         (list "^\\(Less time .*\\): " '(1 'underline))
         (list "^\\(Tags report\\):$" '(1 'underline))
         (list "^\\(Pending goals\\):$" '(1 'underline))
-        (list (concat ett-graph-icon "+") '(0 'ett-separator-face)))
+        (list (concat (string ett-graph-icon) "+") '(0 'ett-separator-face)))
   "Keywords for syntax highlighting on ETT report buffer.")
 
 (defvar ett-items-internal nil)
@@ -269,41 +269,36 @@ Here are some translations (*starred* values are current 
at the time):
 3-2      ---> 03-02-*23*
 03       ---> 03-*02*-*23*
 3        ---> 03-*02*-*23*"
-  (pcase (length date)
-    ;; DD-MM-YY
-    (8 date)
-    ;; DD-MM
-    (5 (setq date (concat date (format-time-string "-%y"))))
-    ;; DD
-    (2 (setq date (concat date (format-time-string "-%m-%y"))))
-    ;; D
-    (1 (setq date (concat "0" date (format-time-string "-%m-%y"))))
-    ;; DD-M
-    (4
-     (setq date
-           ;; Or D-MM?
-           (pcase (string-search "0" date)
-             (2 (concat "0" date (format-time-string "-%y")))
-             (_ (concat
+  (setq date
+       (pcase (length date)
+         ;; DD-MM-YY
+         (8 date)
+         ;; DD-MM
+         (5 (concat date (format-time-string "-%y")))
+         ;; DD
+         (2 (concat date (format-time-string "-%m-%y")))
+         ;; D
+         (1 (concat "0" date (format-time-string "-%m-%y")))
+         ;; DD-M
+         (4
+          ;; Or D-MM?
+          (pcase (string-search "0" date)
+            (2 (concat "0" date (format-time-string "-%y")))
+            (_ (concat
                  (replace-regexp-in-string "-\\(.*\\)" "-0\\1" date)
-                 (format-time-string "-%y"))))))
-    ;; D-M
-    (3 (setq date
-             (concat "0" (replace-regexp-in-string "-\\(.*\\)" "-0\\1" date)
-                     (format-time-string "-%y"))))
-    ;; D-MM-YY
-    (7 (setq date
-             ;; Or DD-M-YY?
-             (pcase (string-search "0" date)
-               (2 (concat "0" date))
-               (_ (concat
-                   (replace-regexp-in-string "-\\(.*\\)-" "-0\\1-" date))))))
-    ;; D-M-YY (who writes like that?)
-    (6 (setq date
-             (concat "0"
-                     (replace-regexp-in-string "-\\(.*\\)-" "-0\\1-" date)))))
-  ;; Return date
-  date)
+                 (format-time-string "-%y")))))
+         ;; D-M
+         (3 (concat "0" (replace-regexp-in-string "-\\(.*\\)" "-0\\1" date)
+                    (format-time-string "-%y")))
+         ;; D-MM-YY
+         (7 ;; Or DD-M-YY?
+          (pcase (string-search "0" date)
+            (2 (concat "0" date))
+            (_ (concat
+                (replace-regexp-in-string "-\\(.*\\)-" "-0\\1-" date)))))
+         ;; D-M-YY (who writes like that?)
+         (6 (concat "0"
+                    (replace-regexp-in-string "-\\(.*\\)-" "-0\\1-" date))))))
 
 (defun ett-custom-date (date &optional date2)
   "Operate on current buffer so the only date is DATE.
@@ -329,7 +324,7 @@ With optional DATE2, include every date between DATE and 
DATE2."
         (ett-finalize-scope "00:00")))))
 
 (defun ett-do-division (num1 num2)
-  "Divide NUM1 between NUM2."
+  "Divide NUM1 between NUM2."          ;can you explain why this is needed?
   (if (/= (% num1 num2) 0)
       (/ (float num1) num2)
     (/ num1 num2)))
@@ -511,7 +506,7 @@ Like `insert-rectangle', but doesn't set mark."
           (insert
            (mapconcat #'concat (make-list (- col (current-column)) " ") "")))
         (ett-insert-rectangle
-         (make-list graph-f ett-graph-icon)))
+         (make-list graph-f (string ett-graph-icon))))
       (setq list (cdr list)
             col (current-column)))
     (setq beacon (point)
@@ -545,7 +540,7 @@ Like `insert-rectangle', but doesn't set mark."
 (defun ett-prettify-time (time)
   "Return a prettified string for TIME."
   (let ((hr 0))
-    (while (>= time 60)
+    (while (>= time 60)                        ;the loop here shouldn't be 
necessary, you can calculate the same thing using remainder and floor
       (setq hr (1+ hr)
             time (- time 60)))
     (concat
@@ -789,7 +784,7 @@ When NOTOTAL is non-nil, don't add total indicator."
     (save-excursion
       ;; Sort items
       ;; We need to do it in two steps because it's somehow complicated
-      (sort-regexp-fields t "^.*$" "\\(:[0-9]+\\)" (point-min) (point-max))
+      (sort-regexp-fields t "^.*$" "\\(:[0-9]+\\)" (point-min) (point-max)) 
;have you considered using rx?
       (sort-regexp-fields t "^.*$" "\\([0-9]+:\\)" (point-min) (point-max))
       ;; We now align
       (align-regexp
@@ -1174,13 +1169,27 @@ Use the command `ett-modeline-mode' to change this 
variable.")
     ;; Clock in
     (ett-add-track item tag)))
 
+(defvar ett-view-mode-map
+  (let ((map (make-sparse-keymap)))
+    (define-key map (kbd "C-c C-c") #'ett-add-track)
+    (define-key map (kbd "SPC") #'ett-space-dwim)
+    map))
+
 (define-derived-mode ett-mode text-mode "ETT"
   "Major mode for editing ETT based texts."
   (setq font-lock-defaults '(ett-font-lock-keywords t))
-  (setq-local outline-regexp "[0-9]+-[0-9]+-[0-9]+")
+  (setq-local outline-regexp "[0-9]+-[0-9]+-[0-9]+") ;does this need to start 
with a ^
   ;; show-paren-mode can be somehow annoying here
   (show-paren-local-mode -1))
 
+(defvar ett-view-mode-map
+  (let ((map (make-sparse-keymap)))
+    (define-key map (kbd "q") #'bury-buffer)
+    (define-key map (kbd "SPC") #'ett-choose-view)
+    (define-key map (kbd "<up>") #'scroll-down-command)
+    (define-key map (kbd "<down>") #'scroll-up-command)
+    map))
+
 (define-derived-mode ett-view-mode ett-mode "ETT [report]"
   "Major mode for viewing ETT reports."
   (setq font-lock-defaults '(ett-view-font-lock-keywords t))
@@ -1188,15 +1197,8 @@ Use the command `ett-modeline-mode' to change this 
variable.")
   (setq-local view-read-only nil)
   (setq cursor-type nil))
 
-(define-key ett-view-mode-map (kbd "q") #'bury-buffer)
-(define-key ett-view-mode-map (kbd "SPC") #'ett-choose-view)
-(define-key ett-view-mode-map (kbd "<up>") #'scroll-down-command)
-(define-key ett-view-mode-map (kbd "<down>") #'scroll-up-command)
-(define-key ett-mode-map (kbd "C-c C-c") #'ett-add-track)
-(define-key ett-mode-map (kbd "SPC") #'ett-space-dwim)
-
 ;;;###autoload
-(add-to-list 'auto-mode-alist (cons (purecopy "\\.ett\\'") 'ett-mode))
+(add-to-list 'auto-mode-alist '("\\.ett\\'" . ett-mode))
 
 (provide 'ett)