Re: [ELPA] New package: SachaC-news

[Philip Kaludercic <philipk@posteo.net>]

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