Re: [ELPA] New package: SachaC-news

[Christian <cnngimenez@disroot.org>]

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

Hi Philip!

Sorry, I misunderstood your idea. What would you like to
discuss specifically?

I will try to pinpoint some things you suggested that is nice
to talk about. The diff has many modifications, and I find it
difficult to see the exact places you want to discuss. In
fact, most of them are changes that I think are better in the
way you wrote.

Below are extracts from the diff and my comments. Tell me if
this method is a good idea to talk about them.

Thanks!
Cheers!

1)
> (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

I think it is correct to remove the :group. I tend to specify
it in case I create more than one customization group, but in
this case, only one is defined.

2)
> +(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'.")

Yep, this variable should be internal. It is used to store the
timestamp when the timer has been activated. And later, the
variable is used to calculate how much time has been passed
since the activation.

Some of the rest of the variables, I believe they should be
internal too.

3)
>  (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))))

I prefer using the expand-file-name, they are more
portable. Did not know that this function exists.

4)
>  (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")

The str variable was used to insert the last new string. The
portion of the Org-mode text with the last title.

But now I changed this function to support diferent formats (txt,
html, org, etc.). This code changed in the current version.

5)
> @@ -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."

Mmm... to my eyes it seems that it does the same but it may be
something I do not know... or maybe I am missing something?
Can I ask you why did you change it? Is the new code a
more convenient or accepted way to do what is intended?
I wonder if perhaps is a parameter or something I do not know
what it does...
Maybe is efficiency: the data is directly printed to the buffer
without transforming into a string?

6)
> @@ -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
Yes, that could be a good idea... However, it should not be a
large string, because it will be displayed on the
minibuffer. Mmm... maybe it is already large...

What do you think? should the string be formatted in a
temporary buffer?

This string is shown when using M-x
sachac-news-show-update-time when an update has been executed
before.

7)
> @@ -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)))

This refers to the sachac-news--git-update internal function.

The sachac-news.el is already using timers, in fact, it must be
a timer the one that at some point calls this function. Also,
there are interactive commands that can trigger a new
git-update. I think it may be more complex to create another
timer inside one to just do the git clone/pull.

Using vc-git it's a good idea! I didn't thought about that!

8)
> @@ -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


Oh! That's true! I wanted to activate the timer
automatically. But it is better to add a comment on the
README.org about this. Thanks!

On Sat, 18 Nov 2023 18:10:24 -0300,
Philip Kaludercic wrote:
>
> Christian <cnngimenez@disroot.org> writes:
>
> > Thanks Philip!
> >
> > I applied the diff to this commit:
> >
> > https://git.sr.ht/~cngimenez/sachac-news/commit/8263dbc7982f543f673172c4a60d4bb68a48c6f6
>
> It appears you applied my comments as well?  I should have made it
> clear, that my message just intended to propose some changes,
> demonstrate possible alternatives and raise some questions for us to
> discuss.
>
> > Cheers!
> > Christian.
> >
> > --
> >
> > - Mastodon: @cnngimenez@mastodon.social
> >
> >  ,= ,-_-. =.  Utilice GPG:
> > ((_/)o o(\_)) * https://emailselfdefense.fsf.org/
> >  `-'(. .)`-'  * Usando la terminal GNU/Linux:
> >      \_/        $ gpg2 --search-keys 77A56F0DA5DD9E05
> >
>
> --
> Philip Kaludercic



--

- Mastodon: @cnngimenez@mastodon.social

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

[-- Attachment #2: OpenPGP Digital Signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]