emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
From: Mario Frasca <mario@anche.no>
To: Bastien <bzg@gnu.org>
Cc: emacs-orgmode@gnu.org
Subject: Re: issue tracker?
Date: Mon, 1 Jun 2020 11:28:48 -0500	[thread overview]
Message-ID: <da7893fe-1400-ca7b-afed-3ae77e8bab4d@anche.no> (raw)
In-Reply-To: <874kruzssr.fsf@gnu.org>

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



  reply	other threads:[~2020-06-01 16:29 UTC|newest]

Thread overview: 71+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-18 21:24 issue tracker? Anthony Carrico
2020-05-18 22:21 ` Nick Dokos
2020-05-18 23:13   ` James R Miller
2020-05-19  7:33     ` tomas
2020-05-19 14:02       ` James R Miller
2020-05-19 14:05         ` James R Miller
2020-05-19 14:53           ` tomas
2020-05-19 14:58       ` Bruce D'Arcus
2020-05-19 16:45         ` Timothy
2020-05-19 16:57         ` Russell Adams
2020-05-19 17:03           ` Timothy
2020-05-19 17:29             ` Russell Adams
2020-05-19 18:50               ` James R Miller
2020-05-19 19:42                 ` Eric Abrahamsen
2020-05-19 20:17                   ` Roland Everaert
2020-05-19 20:47                     ` Diego Zamboni
2020-05-19 21:28                     ` Eric Abrahamsen
2020-05-19 19:48                 ` Russell Adams
2020-05-19 20:14                   ` Trey Ethan Harris
2020-05-19 20:57                   ` gyro funch
2020-05-19 23:22                     ` James R Miller
2020-05-20  9:22           ` Eric S Fraga
2020-05-20  9:40             ` Detlef Steuer
2020-05-20 11:12               ` Stefan Nobis
2020-05-20 16:41                 ` Jud Taylor
2020-05-20 18:55                   ` gennady.uraltsev
2020-05-20 22:05                     ` Bob Newell
2020-05-21  8:10               ` Nicolas Goaziou
2020-05-21 11:21                 ` Bruce D'Arcus
2020-05-21 14:46                 ` Kévin Le Gouguec
2020-05-21 16:31                   ` Eric Abrahamsen
2020-05-22  8:17                     ` Roland Everaert
2020-05-22 14:53                       ` Anthony Carrico
2020-05-23 12:57                         ` Roland Everaert
2020-05-23 13:14                           ` Russell Adams
2020-05-25 11:20                             ` Roland Everaert
2020-05-26 12:34                               ` Robert Pluim
2020-06-01 14:40                       ` Bastien
2020-06-01 14:36                     ` Bastien
2020-05-26 19:17               ` Matthew Lundin
2020-06-01 14:43                 ` Bastien
2020-05-27 17:59         ` Mario Frasca
2020-05-27 18:12           ` Russell Adams
2020-05-27 18:48             ` Eric Abrahamsen
2020-05-31  8:49           ` Russell Adams
2020-06-01 14:45           ` Bastien
2020-06-01 15:46             ` Mario Frasca
2020-06-01 15:53               ` Bastien
2020-06-01 16:28                 ` Mario Frasca [this message]
2020-06-01 16:54                   ` Russell Adams
2020-06-02 11:57                   ` Bastien
2020-06-05 22:44                     ` Mario Frasca
2020-06-06  7:57                       ` Bastien
2020-06-06 16:15                         ` Mario Frasca
2020-06-07  9:38                           ` Bastien
2020-06-07 13:50                             ` Mario Frasca
2020-06-08  9:11                               ` Bastien
2020-05-21  2:35 ` Anthony Carrico
2020-05-21  3:12   ` James R Miller
2020-05-21  5:33     ` Russell Adams
2020-05-21  7:31     ` Kévin Le Gouguec
2020-05-21 14:18       ` Anthony Carrico
2020-05-21 14:38         ` tomas
2020-05-21 14:38         ` Anthony Carrico
2020-05-21 15:05           ` Kévin Le Gouguec
2020-05-22 16:56   ` Ken Mankoff
2020-05-26 19:36   ` Matthew Lundin
2020-06-01 14:59 ` Bastien
2020-09-14  5:23   ` Bastien
  -- strict thread matches above, loose matches on Subject: below --
2020-06-02 11:38 Vladimir Nikishkin
2020-06-02 11:55 ` Bastien

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.orgmode.org/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=da7893fe-1400-ca7b-afed-3ae77e8bab4d@anche.no \
    --to=mario@anche.no \
    --cc=bzg@gnu.org \
    --cc=emacs-orgmode@gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).