From: Nicolas Goaziou <mail@nicolasgoaziou.fr>
To: Florian Beck <fb@miszellen.de>
Cc: emacs-orgmode@gnu.org
Subject: Re: [PATCH] org-table-beginning/end-of-field
Date: Mon, 08 Sep 2014 17:30:16 +0200 [thread overview]
Message-ID: <87wq9e9lkn.fsf@nicolasgoaziou.fr> (raw)
In-Reply-To: <87egvme27x.fsf_-_@sophokles.streitblatt.de> (Florian Beck's message of "Mon, 08 Sep 2014 14:17:06 +0200")
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
next prev parent reply other threads:[~2014-09-08 15:29 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-09-08 5:37 [BUG] org-table-beginning/end-of-field Florian Beck
2014-09-08 10:25 ` Nicolas Goaziou
2014-09-08 12:17 ` [PATCH] org-table-beginning/end-of-field Florian Beck
2014-09-08 15:30 ` Nicolas Goaziou [this message]
2014-09-09 11:09 ` Florian Beck
2014-09-10 13:42 ` Nicolas Goaziou
2014-09-10 17:08 ` Florian Beck
2014-09-12 8:29 ` Nicolas Goaziou
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.orgmode.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87wq9e9lkn.fsf@nicolasgoaziou.fr \
--to=mail@nicolasgoaziou.fr \
--cc=emacs-orgmode@gnu.org \
--cc=fb@miszellen.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).