Thanks Philip!

I applied the diff to this commit:

https://git.sr.ht/~cngimenez/sachac-news/commit/8263dbc7982f543f673172c4a60d4bb68a48c6f6

Cheers!
Christian.

On Fri, 17 Nov 2023 04:28:35 -0300, Kaludercic wrote:
>
> [1  <text/plain (7bit)>]
> 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:
>
> [2  <text/plain (7bit)>]
> 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
> [3  <text/plain (7bit)>]
>
> > 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!!"

--

- Mastodon: @cnngimenez@mastodon.social

 ,= ,-_-. =.  Utilice GPG:
((_/)o o(\_)) * https://emailselfdefense.fsf.org/
 `-'(. .)`-'  * Usando la terminal GNU/Linux:
     \_/        $ gpg2 --search-keys 77A56F0DA5DD9E05