[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [ELPA] New package: SachaC-news
From: |
Philip Kaludercic |
Subject: |
Re: [ELPA] New package: SachaC-news |
Date: |
Sat, 25 Nov 2023 22:50:23 +0000 |
Christian <cnngimenez@disroot.org> writes:
> 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.
It is entirely fine; I'll just be responding to the parts of the message
where I have something constructive to add.
> 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.
No, that was my bad, I must have replaced the variable with a constant
while testing and forgot to change it back.
> But now I changed this function to support diferent formats (txt,
> html, org, etc.). This code changed in the current version.
Would it be worth checking out the code again?
> 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?
Yes; The point of these two changes is to avoid generating a string,
that is immediately discarded (less GC), and to avoid generating a
message when writing the buffer contents to disk.
> 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?
It just seemed like it would be more readable, than having a multi-line
format-string.
> This string is shown when using M-x
> sachac-news-show-update-time when an update has been executed
> before.
Perhaps `display-message-or-buffer' could be of interest?
--
Philip Kaludercic