Re: [ELPA] New package: SachaC-news

[Philip Kaludercic <philipk@posteo.net>]

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

Christian <cnngimenez@disroot.org> writes:

> Hi!
>
> I want to propose SachaC-news (or sachac-news.el if you like)
> package to be included in ELPA. Its objective is to check for
> Sacha Chua's news repository periodically, and to show the Org
> file if there is a new commit with a new post in it. It has
> some customizations too, such as folding specific sections
> automatically, and desktop notifications via "notify-send". The
> requirement is the git program to be installed on your system.
> This information and its usage is at the README.org file at the
> package repository:
>
>           https://github.com/cnngimenez/sachac-news
>
> The code has been checked with byte-compile-file, and
> flycheck configured with checkdoc and flycheck-package [1].

> They do not display any warnings up to commit d00e629, but tell
> me if you find something to fix or any suggestions.

I found a few things, here is a diff with some comments and suggestions:


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

diff --git a/sachac-news.el b/sachac-news.el
index 8d67911..1f389b2 100644
--- a/sachac-news.el
+++ b/sachac-news.el
@@ -22,7 +22,6 @@
 ;; You should have received a copy of the GNU General Public License
 ;; along with this program.  If not, see <https://www.gnu.org/licenses/>.
 
-
 ;;; Commentary:
 
 ;; Check periodically for new commits on Sacha Chua's news repository.
@@ -58,29 +57,29 @@
 
 ;;; Code:
 
-(provide 'sachac-news)
 (require 'org-element)
 (require 'org-list)
-(require 'cl-extra)
+(require 'cl-lib)
 
 (defgroup sachac-news nil
   "Sacha Chua's Emacs news customizations."
   :group 'applications)
 
-(defcustom sachac-news-git-command "git"
+(defcustom sachac-news-git-command
+  (eval-when-compile
+    (require 'vc-git)
+    vc-git-program)
   "Path or git command name.
 
 Valid values are \"/usr/bin/git\" or \"git\" if it is in the current PATH."
-  :type 'string
-  :group 'sachac-news) ;; defcustom
+  :type 'string) ;; defcustom
 
 (defcustom sachac-news-fold-category-regexp-list '()
   "A list of regexp strings of the matching categories that should be folded.
 
 The function `sachac-news-fold-categories' use this variable to find
 categories that the user wants to hide."
-  :type '(repeat regexp)
-  :group 'sachac-news) ;; defcustom
+  :type '(repeat regexp)) ;; defcustom
 
 (defcustom sachac-news-alarm-sound-file
   "/usr/share/sounds/freedesktop/stereo/bell.oga"
@@ -88,8 +87,7 @@ categories that the user wants to hide."
 If the value is nil or the file does not exists, the `ding' function is used.
 
 See `sachac-news-default-sound-alarm' function."
-  :type 'file
-  :group 'sachac-news) ;; defcustom
+  :type 'file) ;; defcustom
 
 (defcustom sachac-news-alarm-sound-programs
   '(("mpv" . "--really-quiet %s")
@@ -100,22 +98,20 @@ programs is founded on the system, the `ding' function will be used.  The
 first program founded is used.
 
 This variable is used by `sachac-news-default-sound-alarm' function."
-  :type '(alist :key-type string :value-type string)
-  :group 'sachac-news ) ;; defcustom
+  :type '(alist :key-type string :value-type string)) ;; defcustom
 
 (defcustom sachac-news-alarm-functions-hook
   '(sachac-news-default-notify-alarm
     sachac-news-default-sound-alarm)
   "The alarm functions.
 These functions are called when there are new news."
-  :type 'hook
-  :group 'sachac-news ) ;; defcustom
+  :type 'hook) ;; defcustom
 
 (defconst sachac-news-title-regexp
   "^\\*\\*[[:space:]]+[[:digit:]]+-[[:digit:]]+-[[:digit:]]+[[:space:]]+Emacs news"
-  "Regexp used to find news titles in the index.org file." ) ;; defconst
+  "Regexp used to find news titles in the index.org file.") ;; defconst
 
-(defvar sachac-news-timer-setted-time 0
+(defvar sachac-news-timer-setted-time 0	;perhaps mark these as internal: sachac-news--...
   "At what time the timer has been setted?
 See `sachac-news-set-timer'.")
 
@@ -148,66 +144,67 @@ Else, this variable contains nil.")
 
 If USE-INDEX-ORG is t, then load the index.org file.  Else, use the current
 buffer as if it is the index.org."
-
   (if use-index-org
       (with-temp-buffer
 	(insert-file-contents (sachac-news-git-index-org))
-	(sachac-news-take-last-new nil) )
+	(sachac-news-take-last-new nil))
     (progn
       (goto-char (point-min))
       (search-forward-regexp sachac-news-title-regexp)
       (let ((sachac-news-title (org-element-at-point)))
 	(buffer-substring-no-properties
 	 (org-element-property :begin sachac-news-title)
-	 (org-element-property :end sachac-news-title))))) )
+	 (org-element-property :end sachac-news-title))))))
 
-(defcustom sachac-news-data-directory (concat user-emacs-directory
-					     "sachac/")
+(defcustom sachac-news-data-directory
+  (locate-user-emacs-file "sachac")
   "Where is the data directory?"
-  :type 'directory
-  :group 'sachac-news) ;; defcustom
+  :type 'directory) ;; defcustom
 
-(defcustom sachac-news-data-file "data.el"
+(defcustom sachac-news-data-file "data.eld"
   "The configuration and data file.
 This is where the last updated date and other data is stored."
-  :type 'file
-  :group 'sachac-news) ;; defcustom
+  :type 'file) ;; defcustom
 
 (defcustom sachac-news-git-dirname "git"
   "The directory where the git repository should be cloned."
-  :type 'string
-  :group 'sachac-news)
+  :type 'string)
 
+;; She publishes the news every week around the beginning, why check
+;; every day?
 (defcustom sachac-news-update-hours-wait 24
   "The amount of hours when the git clone/pull must wait before be called.
 
 Default is 24 hours.  Only positive values should be used."
-  :type 'integer
-  :group 'sachac-news ) ;; defcustom
+  :type 'natnum
+  :group 'sachac-news) ;; defcustom
 
 (defun sachac-news-dir-git ()
   "Return the complete git path."
-  (concat sachac-news-data-directory "/" sachac-news-git-dirname) )
+  (expand-file-name  sachac-news-git-dirname sachac-news-data-directory))
 
 (defun sachac-news-dir-datafile ()
   "Return the complete data file path."
-  (concat sachac-news-data-directory "/" sachac-news-data-file) )
-
+  (expand-file-name sachac-news-data-file sachac-news-data-directory))
 
 (defun sachac-news-git-index-org ()
   "Return the index.org path on the git directory."
-  (concat (sachac-news-dir-git) "/emacs-news/index.org") )
+  (expand-file-name
+   "index.org"
+   (expand-file-name
+    "emacs-news"
+    (sachac-news-dir-git))))
 
 (defun sachac-news--show-last-new-internal ()
   "Show the last news.
 This is used after the update sentinel is executed.
 See `sachac-news-show-last-new'."
-  (let ((str (sachac-news-take-last-new t)))
+  (let ((str (sachac-news-take-last-new t))) ;unused!
     (with-current-buffer (get-buffer-create "*last-news*")
       (org-mode)
 
-      (delete-region (point-min) (point-max))
-      (insert str)
+      (erase-buffer)
+      (insert "foo")
       
       (goto-char (point-min))
 	
@@ -217,7 +214,7 @@ See `sachac-news-show-last-new'."
 	(sachac-news-update-last-saved-title)
 	(sachac-news-fold-categories))
 	
-      (display-buffer (current-buffer)))) )
+      (display-buffer (current-buffer)))))
 
 (defun sachac-news-show-last-new-if-new ()
   "Show the last new if there is a new title.
@@ -241,16 +238,12 @@ see `sachac-news-update-hours-wait' variable."
 			  #'sachac-news--show-last-new-internal
 			  #'sachac-news--show-last-new-internal))
 
-;;
-;; --------------------
-;; Last saved title
-;;
+;;; Last saved title
 
 (defun sachac-news-update-last-saved-title ()
   "Save the last title into the data file."
-
   (setq sachac-news-last-saved-title (sachac-news-get-last-title))
-  (sachac-news-save-data) )
+  (sachac-news-save-data))
 
 (defun sachac-news-get-last-title (&optional use-current-buffer)
   "Get the first title founded in the current buffer.
@@ -264,7 +257,7 @@ the last title.  Else, if t, use the current buffer, but remember to call
 	nil t)
     (with-temp-buffer
       (insert (sachac-news-take-last-new t))
-      (sachac-news-get-last-title t))) )
+      (sachac-news-get-last-title t))))
  
 (defun sachac-news-is-there-new-title-p (&optional use-current-buffer)
   "According to the last save, return t when a new post is found.
@@ -284,12 +277,9 @@ last news buffer.  Else, open the index.org and retrieve the last news."
 	     
     (or (null sachac-news-last-saved-title)
 	(not (string-equal last-title
-			   sachac-news-last-saved-title)))) )
+			   sachac-news-last-saved-title)))))
 
-;;
-;; --------------------
-;; Data or config. load/save
-;;
+;;; Data or config. load/save
 
 (defun sachac-news-load-data ()
   "Update variables which values are in the configuration file.
@@ -305,7 +295,7 @@ important variables."
 	(setq sachac-news-last-saved-title
 	      (alist-get 'last-saved-title expr))
 	;; Return the expression loaded
-	expr))) )
+	expr))))
 
 (defun sachac-news-save-data ()
   "Save some important variables into the data file.
@@ -313,20 +303,17 @@ These variables can be loaded again with `sachac-news-load-data'."
   (with-temp-buffer
     (let ((data (list (cons 'last-update sachac-news-last-update)
 		      (cons 'last-saved-title sachac-news-last-saved-title))))
-    (insert (prin1-to-string data))
-    (write-file (sachac-news-dir-datafile))
-    data)) )
+      (prin1 data (current-buffer))
+      (write-region nil nil (sachac-news-dir-datafile) nil 'silent)
+      data)))
 
 (defun sachac-news-load-data-if-needed ()
   "If the data has not been loaded yet, load it."
   (unless sachac-news-data-loaded
     (sachac-news-load-data)
-    (setq sachac-news-data-loaded t)) )
+    (setq sachac-news-data-loaded t)))
 
-;;
-;; --------------------
-;; Git clone/update
-;;
+;;; Git clone/update
 
 (defun sachac-news-update-last-update ()
   "Update the `sachac-news-last-update' date with the current date."
@@ -335,6 +322,7 @@ These variables can be loaded again with `sachac-news-load-data'."
 
 (defun sachac-news-update-time-str ()
   "Return a string with the last time and the amount of time left."
+  ;; Perhaps format this in a temporary buffer, then return the buffer string?
   (format "Waiting time: %s hours
 -- Update --
 Last time updated: %s
@@ -361,19 +349,19 @@ Time left for automatic forced update: %s %s"
 					     (* sachac-news-update-hours-wait 60 60)))
 	    "No timer setted")
 	  (number-to-string (/ (sachac-news-get-update-time-left) 60))
-	  "minutes") )
+	  "minutes"))
 
 (defun sachac-news-get-update-wait-seconds ()
   "Get the `sachac-news-update-hours-wait' in seconds."
-  (* sachac-news-update-hours-wait 60 60) )
+  (* sachac-news-update-hours-wait 60 60))
 
 (defun sachac-news-show-update-time ()
   "Display the time left for the next update."
   (interactive)
   (sachac-news-load-data-if-needed)
   (if sachac-news-last-update
-      (message (sachac-news-update-time-str))
-    (message "Git has not been called before.")) )
+      (message "%s" (sachac-news-update-time-str))
+    (message "Git has not been called before.")))
 
 (defun sachac-news-get-update-time-left ()
   "Return the seconds left for the next scheduled update.
@@ -384,7 +372,7 @@ been setted)."
   (if sachac-news-timer-setted-time
       (- (+ sachac-news-timer-setted-time (sachac-news-get-update-wait-seconds))
 	 (time-convert (current-time) 'integer))
-    0) )
+    0))
 
 (defun sachac-news-get-update-enable-time-left ()
   "Return the seconds left for the next enabled update.
@@ -398,7 +386,7 @@ loaded)."
   (if sachac-news-last-update
       (- (+ sachac-news-last-update (sachac-news-get-update-wait-seconds))
 	 (time-convert (current-time) 'integer))
-    0) )
+    0))
 
 (defun sachac-news-get-update-time-elapsed ()
   "Return the seconds elapsed since the last update.
@@ -408,19 +396,19 @@ Return the numbre of seconds after the maximum wait + 1 if
   (if sachac-news-last-update
       (- (time-convert (current-time) 'integer)
 	 sachac-news-last-update)
-    (+ (sachac-news-get-update-wait-seconds) 1)) )
+    (+ (sachac-news-get-update-wait-seconds) 1)))
 
 (defun sachac-news-is-time-for-update-p ()
   "Check if a day has passed since the last update."
   (if sachac-news-last-update
       (>= (sachac-news-get-update-time-elapsed)
-	 (sachac-news-get-update-wait-seconds) )
-    t) )
+	 (sachac-news-get-update-wait-seconds))
+    t))
 
 (defun sachac-news-create-dirs ()
   "Create the needed directories to save data and the repository."
   (make-directory sachac-news-data-directory t)
-  (make-directory (sachac-news-dir-git) t) )
+  (make-directory (sachac-news-dir-git) t))
 
 (defun sachac-news--git-sentinel (_process event)
   "Git sentinel.
@@ -454,19 +442,19 @@ FUNC-CALL-AFTER is a function called after the git process endend successfully."
     (when func-call-after
       (add-hook 'sachac-news--git-hook func-call-after))
     (setq sachac-news--git-process
-	  (if (file-exists-p (sachac-news-git-index-org))
-	      (start-process-shell-command "sachac-news-git-pull"
+	  (let ((default-directory (expand-file-name "emacs-news" (sachac-news-dir-git))))
+	    ;; I am not sure what the point is there, but I suspect
+	    ;; there should be a better way to do this using timers
+	    ;; and vc-git.
+	    (if (file-exists-p (sachac-news-git-index-org))
+		(start-process-shell-command "sachac-news-git-pull"
+					     "*sachac-news-git*"
+					     (concat "sleep 60 ; " git-program " pull"))
+	      (start-process-shell-command "sachac-news-git-clone"
 					   "*sachac-news-git*"
-					   (concat
-					    "cd " (sachac-news-dir-git) "/emacs-news ; sleep 60 ; "
-					    git-program
-					    " pull"))
-	    (start-process-shell-command "sachac-news-git-clone"
-					 "*sachac-news-git*"
-					 (concat
-					"cd " (sachac-news-dir-git) "; sleep 60 ; "
-					git-program " clone https://github.com/sachac/emacs-news.git"))))
-    (set-process-sentinel sachac-news--git-process #'sachac-news--git-sentinel)) )
+					   (concat "sleep 60 ; " git-program " clone \
+https://github.com/sachac/emacs-news.git")))))
+    (set-process-sentinel sachac-news--git-process #'sachac-news--git-sentinel)))
 
 
 (defun sachac-news-update-git (&optional force-update
@@ -501,11 +489,11 @@ pull/clone."
 	    (when callback-if-no-update
 	      (funcall callback-if-no-update))))
       ;; Git program not founded
-      (message "%s %s\n%s\n%s"
-	       "The Git program has not been founded!"
-	       "SachaC-news cannot download news without it!"
-	       "Please install it in our system or customize the variable:"
-	       "M-x customize-option sachac-news-git-command"))) )
+      (message (substitute-command-keys
+		"The Git program has not been founded! \
+SachaC-news cannot download news without it!
+Please install it in our system or customize the variable: )
+\\[customize-option] sachac-news-git-command")))))
 
 (defun sachac-news-open-index-file ()
   "Open the index.org file from the local repository.
@@ -519,15 +507,10 @@ how the update is done."
   (sachac-news-update-git)
   (if (file-exists-p (sachac-news-git-index-org))
       (find-file (sachac-news-git-index-org))
-    (message "%s\n%s"
-	     "Index file not found! Did something wrong happen?"
-	     "See `sachac-news-update-git'.")) )
+    (message "Index file not found! Did something wrong happen?
+See `sachac-news-update-git'.")))
 
-
-;;
-;; --------------------
-;; Folding categories
-;;
+;;; Folding categories
 
 (defun sachac-news-find-all-categories (category-regexps &optional org-element)
   "Match paragraph with the CATEGORY-REGEXPS regexp.
@@ -554,7 +537,7 @@ Returns a list of org-element of type \\'item found in the index.org."
 			  (string-match-p category element))
 			category-regexps))
 
-	    parent)))) )
+	    parent)))))
 
 
 (defun sachac-news-fold-all-items (item-list)
@@ -582,12 +565,9 @@ This function works on any Org file, even at the Emacs news' index.org."
   (let ((category-list (if category-regexp-list category-regexp-list
 			 sachac-news-fold-category-regexp-list)))
     (sachac-news-fold-all-items
-     (sachac-news-find-all-categories category-list))) )
+     (sachac-news-find-all-categories category-list))))
 
-;;
-;; --------------------
-;; Alarm
-;;
+;;; Alarm
 
 (defun sachac-news-default-notify-alarm ()
   "The default alarm.
@@ -596,7 +576,7 @@ Use the notify-send to send the alarm."
     (when program
       (shell-command (concat program
 			     " --app-name=\"Emacs: SachaC-news\""
-			     " \"Check the News!\"")))) )
+			     " \"Check the News!\"")))))
 
 (defun sachac-news-default-sound-alarm ()
   "The default sound alarm.
@@ -619,17 +599,14 @@ as fallback."
 	       (car program-data)
 	       (split-string
 		(format (cadr program-data) sachac-news-alarm-sound-file)))
-      (ding t))) )
+      (ding t))))
 
 (defun sachac-news-run-alarm-if-needed ()
   "Run the alarm hook functions if there is a new post ."
   (when (sachac-news-is-there-new-title-p)
-    (run-hooks 'sachac-news-alarm-functions-hook)) )
+    (run-hooks 'sachac-news-alarm-functions-hook)))
 
-;;
-;; --------------------
-;; Timer
-;;
+;;; Timer
 
 (defun sachac-news-timer-function ()
   "The function used by the timer."
@@ -638,7 +615,7 @@ as fallback."
   (sachac-news-update-git t #'sachac-news-show-last-new-if-new)
   (sachac-news-run-alarm-if-needed)
 
-  (sachac-news-activate-timer) )
+  (sachac-news-activate-timer))
 
 
 (defun sachac-news-activate-timer ()
@@ -650,9 +627,9 @@ Set the timer for executing on `sachac-news-update-hours-wait' hours."
   (setq sachac-news-timer-setted-time (time-convert (current-time) 'integer))
   (setq sachac-news-timer
 	(run-at-time
-	 (concat (number-to-string sachac-news-update-hours-wait) "hours")
-		     nil
-		     #'sachac-news-timer-function)) )
+	 (format "%d hours" sachac-news-update-hours-wait)
+	 nil
+	 #'sachac-news-timer-function)))
 
 (defun sachac-news-deactivate-timer ()
   "Stop and cancel the timer."
@@ -660,7 +637,7 @@ Set the timer for executing on `sachac-news-update-hours-wait' hours."
   (when (timerp sachac-news-timer)
     (cancel-timer sachac-news-timer)
     (setq sachac-news-timer nil))
-  (setq sachac-news-timer-setted-time nil) )
+  (setq sachac-news-timer-setted-time nil))
 
 (defun sachac-news-timer-status ()
   "Is the timer setted or not?
@@ -668,8 +645,13 @@ Report the user about the timer status."
   (interactive)
   (if (timerp sachac-news-timer)
       (message "Timer is setted and running.")
-    (message "Timer is deactivated")) )
+    (message "Timer is deactivated")))
+
+;; Don't activate side effects while loading your package!  Instruct
+;; the users to add this to their init.el, so that one knows what is
+;; going on.
 
-(sachac-news-activate-timer)
+;; (sachac-news-activate-timer)
 
+(provide 'sachac-news)
 ;;; sachac-news.el ends here

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


> According to http://elpa.gnu.org/, and the README.org [2] file
> linked there, it says to notify to this mail address as
> first step. Also, it mentions other steps regarding pushing the
> code and adding an entry on elpa-packages file. But, there is a
> marker above indicating "OUTDATED" [3].

That should be addressed ^^

> Please, could you tell me if these steps are needed?
> If they are not, how should I proceed?

As soon as we sort out the above comments, I can take care of adding the
package for you.

> Cheers!
> Christian Gimenez
>
> [1] https://github.com/purcell/flycheck-package
> [2] https://git.savannah.gnu.org/cgit/emacs/elpa.git/plain/README
> [3] The section states: "Text below this marker is OUTDATED and
> still needs to be reviewed/rewritten!!"