[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [O] [PATCH] org-table-beginning/end-of-field
From: |
Nicolas Goaziou |
Subject: |
Re: [O] [PATCH] org-table-beginning/end-of-field |
Date: |
Mon, 08 Sep 2014 17:30:16 +0200 |
Florian Beck <address@hidden> writes:
> How about this?
Nice. Some comments follow.
>
> From 4fb2bbff2238d15ae7c896e0eb268b74ea4e56dc Mon Sep 17 00:00:00 2001
> From: Florian Beck <address@hidden>
> 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