Re: issue tracker?

[Mario Frasca <mario@anche.no>]

On 01/06/2020 10:53, Bastien wrote:
> Let us know what would help you contribute more.

as mentioned, I would like to correct docstrings, refactor the code in a 
few points, and add unit tests.

---

(defun org-plot/gnuplot-script (data-file num-cols params &optional preface)
   "Write a gnuplot script to DATA-FILE respecting the options set in 
PARAMS.

this is not what the function does: DATA-FILE is purely a string, the 
name of the file containing the data, and the function returns the 
script as a string, which refers to DATA-FILE.

in practice, the author could have left the DATA-FILE parameter 
altogether, used a placeholder, say %DATAFILE%, and replaced it in the 
returned string.  but it works, and I would not fix it, except for the 
docstring, which is misleading.

---

(defun org-plot/goto-nearest-table ()
   "Move the point to the beginning of nearest table.
Moves back if the point is currently inside of table, otherwise
forward to next table.  Return value is the point."

this is what the function does, but the current docstring says

(defun org-plot/goto-nearest-table ()
   "Move the point forward to the beginning of nearest table.
Return value is the point at the beginning of the table."

where "nearest" is not defined.

---

collecting options is a candidate for refactoring:

       (save-excursion (while (and (equal 0 (forward-line -1))
                   (looking-at "[[:space:]]*#\\+"))
             (setf params (org-plot/collect-options params))))

this is how it is used, inside of the exposed command org-plot/gnuplot.

---

If I understood my other reviewer Kyle Meyer correctly, he was 
suggesting that usage of setf instead of setq in this source was not 
exactly appropriate, and I think it is only necessary in one case, the 
others can be replaced with setq.

but then again, fixing something that works and has no unit tests, may 
be a recipe for future failure.

---

there are other minor mistakes in the code and in the documentation, 
which are quite unrelated, like formatting …

       (let ((num-rows (length table)) (num-cols (length (nth 0 table)))
         (gnuplot-row (lambda (col row value)

notice how this let has three clauses, but they fit on two lines.

or simply typing =dep= instead of =deps=.

---

I've not been collecting them, this is just the few that I can recall, 
and would like to correct them, but in order to make my contribution 
only touch the code I want to add, I refrain from doing so.

best regards, MF