From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Christian Newsgroups: gmane.emacs.devel Subject: Re: [ELPA] New package: SachaC-news Date: Sat, 25 Nov 2023 19:07:23 -0300 Message-ID: <87fs0txzac.wl-cnngimenez@disroot.org> References: <87o7fyix8e.wl-cnngimenez@disroot.org> <87leawq1ng.fsf@posteo.net> <875y1yrehf.wl-cnngimenez@disroot.org> <87zfzazs1r.fsf@posteo.net> Mime-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: multipart/signed; boundary="pgp-sign-Multipart_Sat_Nov_25_19:07:23_2023-1"; micalg=pgp-sha256; protocol="application/pgp-signature" Content-Transfer-Encoding: 7bit Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="22347"; mail-complaints-to="usenet@ciao.gmane.io" Cc: Christian , emacs-devel@gnu.org To: Philip Kaludercic Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Sat Nov 25 23:08:56 2023 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1r70pa-0005bm-RZ for ged-emacs-devel@m.gmane-mx.org; Sat, 25 Nov 2023 23:08:54 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1r70ot-00034f-7j; Sat, 25 Nov 2023 17:08:11 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1r70or-00034W-VH for emacs-devel@gnu.org; Sat, 25 Nov 2023 17:08:10 -0500 Original-Received: from layka.disroot.org ([178.21.23.139]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1r70op-0000k5-3x for emacs-devel@gnu.org; Sat, 25 Nov 2023 17:08:09 -0500 Original-Received: from localhost (localhost [127.0.0.1]) by disroot.org (Postfix) with ESMTP id B6D4F41AC2; Sat, 25 Nov 2023 23:08:03 +0100 (CET) X-Virus-Scanned: SPAM Filter at disroot.org Original-Received: from layka.disroot.org ([127.0.0.1]) by localhost (disroot.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id AaZLgFS3Q1fb; Sat, 25 Nov 2023 23:08:02 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=disroot.org; s=mail; t=1700950082; bh=DV+Er/UvwmyI0WbVUQb4cUCt4efPA9q7yD3nQ3T6Ink=; h=Date:From:To:Cc:Subject:In-Reply-To:References; b=W7Vd5TZWXZp38gfeZYpJJSYS8PwN41VorAkQUELy6JRFyFNyo+toXYJWMk3WykI6N BLBpzgbBOyAJdDqbKjeWfOOubC1rmlOdv3+ri4NFtYYHAL9pPej+w+OcOfFvj3T/hX YHx3vE0qlG7mOU6Sro45W46Mc5QPl3Ri/ilha0hVriIbpsi0r+FZiziW93tnwPm6tk ApoKP0DjyAoQlCeiPUPf8Wh4W9wyfk8uzHNqCZH9L2v1BihGCUIzpk3A0scad5Uzpq X9UJwZEYbyCXtmEDSXa3/LbfKdFFLk4AW1OGNR1JLofeEI78XkfgFpudrpL30jarNr f+uihYSkA1Sqg== In-Reply-To: <87zfzazs1r.fsf@posteo.net> X-Face: '*BN,gre{ZP%#f"H?LXo(; RPw0x`+l &(ddXwk]TaK9CK@u_, 1Lfs2T@h\{56yy J'8{Cnz_[mlVz:5-Kte[qt$kH[ List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.devel:313225 Archived-At: --pgp-sign-Multipart_Sat_Nov_25_19:07:23_2023-1 Content-Type: text/plain; charset=US-ASCII 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 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 --pgp-sign-Multipart_Sat_Nov_25_19:07:23_2023-1 Content-Type: application/pgp-signature Content-Transfer-Encoding: 7bit Content-Description: OpenPGP Digital Signature -----BEGIN PGP SIGNATURE----- iHUEABEIAB0WIQTKgp4b7n/ZedVkiop3pW8Npd2eBQUCZWJwGwAKCRB3pW8Npd2e BehgAQCKFd5x4IpAKB/K75skw9E+10fId03bmwX2GTMoeHSB1wD/Xz+Xt2ewNHCo AZ2DkxEAXGb9kAgloOAaCK44j6Gk/80= =v6h1 -----END PGP SIGNATURE----- --pgp-sign-Multipart_Sat_Nov_25_19:07:23_2023-1--