From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Banel Subject: Re: make orgtbl-ascii-plot easier to install Date: Tue, 26 Aug 2014 21:52:20 +0200 Message-ID: <53FCE574.3010307@free.fr> References: <53C2ADB0.2010404@free.fr> <87oaw9wg3s.fsf@bzg.ath.cx> <53E7F088.8010401@free.fr> <87tx4zhcvs.fsf@nicolasgoaziou.fr> Mime-Version: 1.0 Content-Type: text/html; charset=windows-1252 Content-Transfer-Encoding: quoted-printable Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:59487) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XMMn5-0006ba-4u for emacs-orgmode@gnu.org; Tue, 26 Aug 2014 15:52:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XMMn4-0003JW-24 for emacs-orgmode@gnu.org; Tue, 26 Aug 2014 15:52:27 -0400 Received: from smtp4-g21.free.fr ([2a01:e0c:1:1599::13]:11064) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XMMn3-0003GP-OL for emacs-orgmode@gnu.org; Tue, 26 Aug 2014 15:52:26 -0400 Received: from [IPv6:2a01:e35:2e21:def0:f485:fae9:2cf9:1f52] (unknown [IPv6:2a01:e35:2e21:def0:f485:fae9:2cf9:1f52]) by smtp4-g21.free.fr (Postfix) with ESMTP id 156BA4C80CA for ; Tue, 26 Aug 2014 21:52:20 +0200 (CEST) In-Reply-To: <87tx4zhcvs.fsf@nicolasgoaziou.fr> List-Id: "General discussions about Org-mode." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org Sender: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org To: emacs-orgmode@gnu.org

Thanks Nicola= s for your valuable insight. I will apply your recommendations.

Le 26/08/2014 10:29, Nicolas Goaziou a =E9crit=A0:

 (orgtbl-ascii-plot): top-level function=20
 (orgtbl-ascii-draw), 	(orgtbl-uc-draw-grid),=20
 (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 co=
mpose 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!lhVH=
W"))
+  (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=3D'(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:
=A0 "<345"
=A0 "345()"
=A0 "345%45"
=A0 "0x45"=A0=A0 // hexadecimal
=A0 "1101#2"=A0 // 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).=A0

+     (or (memq 'hline table) table)) ;; skip tabl=
e 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.

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