From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nicolas Goaziou Subject: Re: make orgtbl-ascii-plot easier to install Date: Tue, 26 Aug 2014 10:29:59 +0200 Message-ID: <87tx4zhcvs.fsf@nicolasgoaziou.fr> References: <53C2ADB0.2010404@free.fr> <87oaw9wg3s.fsf@bzg.ath.cx> <53E7F088.8010401@free.fr> Mime-Version: 1.0 Content-Type: text/plain Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:44660) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XMC85-0007Vk-1m for emacs-orgmode@gnu.org; Tue, 26 Aug 2014 04:29:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XMC7u-0001rt-Ni for emacs-orgmode@gnu.org; Tue, 26 Aug 2014 04:29:25 -0400 Received: from relay3-d.mail.gandi.net ([2001:4b98:c:538::195]:45803) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XMC7u-0001rd-FF for emacs-orgmode@gnu.org; Tue, 26 Aug 2014 04:29:14 -0400 In-Reply-To: <53E7F088.8010401@free.fr> (Thierry Banel's message of "Mon, 11 Aug 2014 00:22:00 +0200") 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: Thierry Banel Cc: emacs-orgmode@gnu.org Hello, Thierry Banel writes: > Sorry for the late answer, I was on the road. > The patch is attached hereafter. Thank you for the patch. Some comments follow. > From 5fddaba2208c2cb4ce3b6bc24d0d10571124fb39 Mon Sep 17 00:00:00 2001 > From: Thierry Banel > Date: Mon, 11 Aug 2014 00:00:21 +0200 > Subject: [PATCH] * org-table.el: add ascii plotting in tables, > (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'. > +(defun orgtbl-ascii-draw (value min max &optional width characters) > + "Draws an ascii bar in a table. > +VALUE is a the value to plot, the width of the bar to draw. > +A value equal to MIN will be displayed as empty (zero width bar). > +A value equal to MAX will draw a bar filling all the WIDTH. > +WIDTH is the expected width in characters of the column. > +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. > +with shades of grey from pure white to pure black. > +It defaults to a 10 characters string of regular ascii characters. > +" Spurious newline at the end of the docstring. > + (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)))) > + (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. > + (setq value (* (/ (- (+ value 0.0) min) (- max min)) width)) (let ((value ...))) > + (cond > + ((< value 0) "too small") > + ((> value width) "too large") > + (t > + (let ((len (1- (length characters)))) > + (concat > + (make-string (floor value) (elt characters len)) > + (string (elt characters > + (floor (* (- value (floor value)) len))))))))) > + > +(defun orgtbl-uc-draw-grid (value min max &optional width) > + "Draws an ascii bar in a table. > +It is a variant of orgtbl-ascii-draw with Unicode block characters, > +for a smooth display. > +Bars appear as grids (to the extend the font allows). > +" Spurious newline. I wouldn't put the last sentence on its own line, too. Also, isn't it "extent"? > +(defun orgtbl-uc-draw-cont (value min max &optional width) > + "Draws an ascii bar in a table. > +It is a variant of orgtbl-ascii-draw with Unicode block characters, > +for a smooth display. > +Bars are solid (to the extend the font allows). > +" Ditto. > + (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. Spurious newline too. > + (interactive "P") > + (let ((col (org-table-current-column)) > + (min 1e999) `most-positive-fixnum' > + (max -1e999) `most-negative-fixnum' > + (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))))) > + (mapc Small nitpick: I suggest to use `dolist' instead of `mapc' (no funcall overhead). > + (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? > + (setq x (string-to-number x)) > + (if (> min x) (setq min x)) > + (if (< max x) (setq max x))))) (when (> min x) ...) (when (< max x)) > + (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. Regards, -- Nicolas Goaziou