Re: [PATCH] org-table-beginning/end-of-field

[Nicolas Goaziou <mail@nicolasgoaziou.fr>]

Florian Beck <fb@miszellen.de> writes:

> How about this?

Nice. Some comments follow.
>
> From 4fb2bbff2238d15ae7c896e0eb268b74ea4e56dc Mon Sep 17 00:00:00 2001
> From: Florian Beck <fb@miszellen.de>
> Date: Mon, 8 Sep 2014 14:08:56 +0200
> Subject: [PATCH] org-table: fix arguments of `org-table-beginning-of-field'

"fix" needs to be capitalized.

>  and `org-table-end-of-field'. Also fix docstring and cleanup code.

You need a shorter summary, e.g.,

  org-table: Fix org-table/beginning/end/-of-field.

The second sentence should go at the end of the commit message.  Also,
there's no full stop on summary lines.

> * lisp/org-table.el (org-table--border-of-field): new function

"new" needs to be capitalized.  Also there's a missing full stop.

> (org-table-beginning-of-field): call it
> (org-table-end-of-field): call it

(org-table-beginning-of-field, org-table-end-of-field): Call it.

> +(defun org-table--border-of-field (n move-fn element cmp-fn)

A docstring would be nice, even for a helper function.

>    (let ((pos (point)))
>      (while (> n 1)
>        (setq n (1- n))
> -      (org-table-previous-field))
> -    (if (not (re-search-backward "|" (point-at-bol 0) t))
> -	(user-error "No more table fields before the current")
> -      (goto-char (match-end 0))
> -      (and (looking-at " ") (forward-char 1)))
> -    (if (>= (point) pos) (org-table-beginning-of-field 2))))
> +      (funcall move-fn))

While you're at it, I suggest

  (dotimes (_ (1- n)) (funcall move-fn))

instead of

  (while (> n 1) ...)

Also, note that you can avoid requiring MOVE-FN, ELEMENT and CMP-FN if
you decide that

  (< n 0)  => (#'org-table-next-field :contents-end #'<=)
  (> n 0)  => (#'org-table-previous-fierd :contents-begin #'>=)

Up to you.

> +    (goto-char (org-element-property element (org-element-context)))

You need to be more careful here.  You might be outside of a table, or
within an object contained in a cell, e.g.,

  | some *bold* object |

where (org-element-context) on bold will return a `bold' type object
with different :contents-begin and :contents-end values.

IOW, you need to let-bind (org-element-context) and find the first
`table', `table-row' or `table-cell' object/element among it and its
parents. Then

  - if no such ancestor is found: return an error (not at a table)

  - if `table' is found but point is not within
    [:contents-begin :contents-end[ interval, return an error (not
    inside the table)

  - if `table' or `table-row' is found, you need to apply
    org-table/previous/next/-field once (and diminish N by one) to make
    sure point will be left on a regular cell, if possible.

> +    (if (and (> n 0) (funcall cmp-fn (point) pos))
> +	(org-table--border-of-field 2 move-fn element cmp-fn))))

I don't think (> n 0) is necessary, is it? Also, I suggest to use `when'
(or `and' if return value matters) over one-armed `if'.

> -(defun org-table-end-of-field (&optional n)
> +(defun org-table-beginning-of-field (&optional n)
>    "Move to the beginning of the current table field.
> -If already at or before the beginning, move to the beginning of the
> -previous field.
> +If already at or before the beginning and N is not 0, move to the 
> +beginning of the previous table field.
>  With numeric argument N, move N-1 fields backward first."

I wouldn't start a new line here.  Also don't forget double spaces:

  If already at or before the beginning and N is not 0, move to the
  beginning of the previous table field.  With numeric argument N, move
  N-1 fields backward first.

> +  (org-table--border-of-field (or n 1)
> +			      'org-table-previous-field
> +			      :contents-begin '>=))

Nitpick: #'org-table-previous-field and #'>=.

> +(defun org-table-end-of-field (&optional n)
> +  "Move to the end of the current table field.
> +If already at or after the end and N is not 0, move to the end of the
> +next field.
> +With numeric argument N, move N-1 fields forward first."
> +  (interactive "p")
> +  (org-table--border-of-field (or n 1)
> +			      'org-table-next-field
> +			      :contents-end '<=))

Ditto.

Thank you for taking care of this. There are bonus points if you can
write tests along with this change.


Regards,

-- 
Nicolas Goaziou