Re: [NonGNU Elpa] New package: ETT

[Philip Kaludercic <philipk@posteo.net>]

John Task <q01@disroot.org> writes:

>>  (defun ett-do-division (num1 num2)
>> -  "Divide NUM1 between NUM2."
>> +  "Divide NUM1 between NUM2."		;can you explain why this is needed?
>>    (if (/= (% num1 num2) 0)
>>        (/ (float num1) num2)
>>      (/ num1 num2)))
>
> I need the extra precision for various functions.  (/ 12 7) gives me
> 1, while (ett-do-division 12 7) gives me 1.7142857142857142.

If that is so, I would explain that in the documentation string, but
what I wonder is why if condition is needed.  All you basically do is
avoid converting num1 to a float, and the result remains a integer, in
some cases.

>>  (defun ett-prettify-time (time)
>>    "Return a prettified string for TIME."
>>    (let ((hr 0))
>> -    (while (>= time 60)
>> +    (while (>= time 60)			;the loop here shouldn't be necessary, you can calculate the same thing using remainder and floor
>>        (setq hr (1+ hr)
>>              time (- time 60)))
>
> I'll look into that.  Is there any reason why I would need to avoid
> that loop?  The code just works for now.

While this will calculate the values in constant time,

  (list (floor time 60) (% time 60))

a loop will always take more iterations depending on the value of the input:

  (let ((hr 0))
    (while (>= time 60)
      (setq hr (1+ hr)
             time (- time 60)))
    (list hr time))

>>      (save-excursion
>>        ;; Sort items
>>        ;; We need to do it in two steps because it's somehow complicated
>> -      (sort-regexp-fields t "^.*$" "\\(:[0-9]+\\)" (point-min) (point-max))
>> +      (sort-regexp-fields t "^.*$" "\\(:[0-9]+\\)" (point-min) (point-max)) ;have you considered using rx?
>>        (sort-regexp-fields t "^.*$" "\\([0-9]+:\\)" (point-min) (point-max))
>>        ;; We now align
>>        (align-regexp
>
> I have considered it, but while my regexps are weird, they are
> still readable for now.  Maybe I'll reconsider it.

FWIW i even hesitated in bringing this up, because as you say the
regular expressions are currently modest in complexity.

>> +(defvar ett-view-mode-map
>> +  (let ((map (make-sparse-keymap)))
>> +    (define-key map (kbd "C-c C-c") #'ett-add-track)
>> +    (define-key map (kbd "SPC") #'ett-space-dwim)
>> +    map))
>
> I think you mean just ett-mode-map.  In that case, OK.

Right, that was a typo.  All I provide is faulty static analysis ^^

>> (define-derived-mode ett-mode text-mode "ETT"
>>    "Major mode for editing ETT based texts."
>>    (setq font-lock-defaults '(ett-font-lock-keywords t))
>> -  (setq-local outline-regexp "[0-9]+-[0-9]+-[0-9]+")
>> +  (setq-local outline-regexp "[0-9]+-[0-9]+-[0-9]+") ;does this need to start with a ^
>>    ;; show-paren-mode can be somehow annoying here
>>    (show-paren-local-mode -1))
>
> Documentation of the variable says it isn't necessary, and it worked
> as is on my tests, but I'm not an expert.

OK, I didn't know about that, then this is fine.