Re: [O] make orgtbl-ascii-plot easier to install

[Thierry Banel]

Thanks Nicolas for your valuable insight. I will apply your recommendations.

Le 26/08/2014 10:29, Nicolas Goaziou a écrit :

 (orgtbl-ascii-plot): top-level function 
 (orgtbl-ascii-draw), 	(orgtbl-uc-draw-grid), 
 (orgtbl-uc-draw-cont): helper functions which go in table

If they are helper functions, I suggest to use "--" in their name, e.g.,
`orgbtl--ascii-draw'.

Well, all those functions have user-visibility. They appear in the #+TBLFM: line. I should avoid the words "helper function" here.

+CHARACTERS is a string of characters that will compose the bar,

This is a minor issue, but a "string of characters" sounds strange:
aren't all strings constituted of characters? Maybe you really want
a list of characters.

Yes :) Just string.

+  (unless characters (setq characters " .:;c!lhVHW"))
+  (unless width (setq width 12))

I suggest let-binding variables instead:

  (let ((characters (or characters " .:;c!lhvHW"))
        (width (or width 12))))

I'll change to let-binding. What is the rational for that ? Better byte-code ?

+  (if (stringp value)
+      (setq value (string-to-number value)))

Prefer `and' or `when' over one-armed `if'.  Also, this may be dangerous
since `string-to-number' can return funny values.  Why wouldn't you
simply forbid strings and limit VALUE to integers.

Can I limit VALUE to numbers ? VALUE comes from a cell in the table. The formula line of the table looks like this:

#+TBLFM: $2='(orgtbl-ascii-draw $1 1 3 12)

VALUE is $1 in this formula.
If (string-to-number VALUE) fails, it will return zero, which is not very harmful. But yes, you are right, it would be better to check funny values, and explicitly do something sensible in this case.

+  (orgtbl-ascii-draw value min max width " \u258F\u258E\u258D\u258C\u258B\u258A\u2589\u2588"))
+
+;;;###autoload
+(defun orgtbl-ascii-plot (&optional ask)
+  "Draws an ascii bars plot in a column, out of values found in another column.
+A numeric prefix may be given to override the default 12 characters wide plot.
+"

You must refer explicitly to ASK in your docstring. In particular, you
may want to detail the distinction between '(4) and 4.

Absolutely.

+  (interactive "P")
+  (let ((col (org-table-current-column))
+	(min  1e999)

`most-positive-fixnum'

Actually this should be `cl-most-positive-float', because everything here works in floating point values. Thanks for the clue.

+	(length 12)
+	(table (org-table-to-lisp)))
+    (cond ((consp ask)
+	   (setq length
+		 (or
+		  (read-string "Length of column [12] " nil nil 12)
+		  12)))
+	  ((numberp ask)
+	   (setq length ask)))

(let ((length (cond ((consp ask)
                     (read-number "Length of column [12] " nil nil 12))
                    ((numberp ask) ask)
                    (t 12)))))

Ok, shorter.

+    (mapc

Small nitpick: I suggest to use `dolist' instead of `mapc' (no funcall
overhead).

Sure, easier to read.

+     (lambda (x)
+       (when (consp x)
+	 (setq x (nth (1- col) x))
+	 (when (string-match
+		"^[-+]?\\([0-9]*[.]\\)?[0-9]*\\([eE][+-]?[0-9]+\\)?$"
+		x)

Would `org-table-number-regexp' make sense here instead of the
hard-coded regexp?

Before hard-coding this regexp, I took a look at what was already there. org-table-number-regexp matches too much, for instance it matches:
  "<345"
  "345()"
  "345%45"
  "0x45"   // hexadecimal
  "1101#2"  // base two
Anyway, the string feeds (string-to-number x) which does not accept all those variations. So I created a regexp exactly fitted for (string-to-number). 

+     (or (memq 'hline table) table)) ;; skip table header if any

This check is not sufficient in the following cases:

  |-----------|
  | no header |
  |-----------|

and

  |----------|
  |  header  |
  |----------|
  | contents |

IOW, you need to eliminate the leading 'hline, if any,and skip until
the next 'hline if there is one and if there is something after it.

Ok. Not completely fool-proof, but better. On the other hand, this loop searches for the min and the max of a column, ignoring entries which are not numbers. So it could just iterate over the full column, including any kind of headers.

  | header | <-- ignored
  |--------| <-- ignored
  |      1 | <-- used (will set min=1)
  |      2 | <-- used
  |     xx | <-- ignored
  |      3 | <-- used (will set max=3)
  |--------| <-- ignored