[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [O] [patch, ox] #+INCLUDE resolves links

From: Rasmus
Subject: Re: [O] [patch, ox] #+INCLUDE resolves links
Date: Sun, 28 Sep 2014 21:32:08 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.4.50 (gnu/linux)


Thanks for the comments.  I hope I addressed the previous comments and
did not introduce new reasons bugs.
I added tests.

Comments on comments follow.

Nicolas Goaziou <address@hidden> writes:

>> Okay, I hope I got it now.  It's a rather forgiving regexp in terms of
>> mistakes.  Is that OK?
> Please no ":only-contents yes", ":only-contents true", ":only-contents
> of_course!" in the regexp. If :only-contents is followed by anything but
> nil or another keyword, its value is non-nil. See below.

Good catch; I added explicit support for


in the regexp!  ¡Gotta catch 'em all!

> The sentence is not complete. Also, it should be something like "If you
> set @code{:only-contents} property to a non-nil value, only...".
>> [...]
> This is not true anymore about the drawers. This should be merged with
> the previous phrase to avoid duplicating "@code{:only-contents}" (e.g.,
> only the contents of the matched element are inserted, without any
> planning line or property drawer).

Fixed this and other documentation bugs — hopefully.  Let me know if
it's clear.

>> +                        (let ((matched (save-match-data
>> +                                         (org-split-string
>> + (org-remove-double-quotes (match-string 1 value)) "::"))))
> There's no reason to use `org-split-string' here since you only want to
> match the last "::". You can use the same regexp used by
> "org-element.el", i.e.
>   (when (string-match "::\\(.*\\)\\'" value)
>     (setq location (match-string 1 value)
>           value (replace-match "" nil nil value)))

 Custom_ID is very flexible.  I've use a similar regexp.

>> +             (only-contents
>> +              (and (string-match
>> + ":only-?contents?[[:space:]]*\"?\\(t\\|true\\|yes\\)?\"?"
>> +                    value)
>> +                   (prog1 (and (match-string 1 value) t)
>> +                     (setq value (replace-match "" nil nil value)))))
>   (only-contents
>    (and (string-match ":only-contents +\\([^: \r\t\n]\\S-*\\)" value)
>         (org-not-nil (match-string 1 value))))

I have removed flexibility in speling.

>> +      (narrow-to-region
>> +       (org-element-property
>> +    (if only-contents :contents-begin :begin) (org-element-at-point))
>> + (org-element-property (if only-contents :contents-end :end)
>> (org-element-at-point))))
>   (let ((element (org-element-at-point)))
>     (let ((contents-beg
>            (and only-contents
>                 (org-element-property :contents-begin element))))
>       (narrow-to-region
>        (or contents-beg (org-element-property :begin element))
>        (org-element-property (if contents-beg :contents-end :end) element))))

Just out of curiosity, what is an example of a element that can be
named and does not have a :contents-begin?

>> +    (when only-contents
>> +      ;; skip drawers and property-drawers
>> +      ;; these are removed as needed in `org-export--prepare-file-contents'
>> +      ;; TODO: How to actually do this?  Only line numbers are send to
>> +      ;; `org-export--prepare-file-contents'.  split in two?
>> +      (goto-char (point-min))
>> +      (org-skip-whitespace)
>> +      (beginning-of-line)
>> +      (let ((element (org-element-at-point)))
>> +    (while (memq (org-element-type element) '(drawer property-drawer))
>> +      (goto-char (org-element-property :end element))
>> +      (setq element (org-element-at-point)))))
> Regular drawers are not expected to be skipped. Also, the following
> should be better
>   (when (and only-contents
>              (memq (org-element-type element) '(headline inlinetask)))
>     (goto-char (point-min))
>     (when (org-looking-at-p org-planning-line-re) (forward-line))
>     (when (looking-at org-property-drawer-re) (goto-char (match-end 0)))
>     (unless (bolp) (forward-line)))
> This should be obviously included within the previous `let'.

Okay, there's a lot of improvements in that suggestion.  However, it
misses this case which created using only "official" shortcuts

     * head
     SCHEDULED: <2014-09-28 sun>
     - Note taken on [2014-09-28 sat 12:21] \\
       a drawer
     :CUSTOM_ID: h

The patch handles something like this now cf. the last test.

>> +    (apply (lambda (beg end) (format "%s-%s" beg end))
>> +       ;; `line-number-at-pos' returns the narrowed line-number
>> + (mapcar 'line-number-at-pos (prog1 (list (point-min) (point-max))
>> (widen))))))
> This is inefficient because `line-number-at-pos' will start counting
> twice from line 1.
>   (goto-char beg)
>   (widen)
>   (let ((start-line (line-number-at-pos)))
>     (format "%d-%d"
>             start-line
>             (+ start-line
>                (let ((c 0)) (while (< (point) end) (incf c) (forward-line)) 
> c))))
> I didn't check, there may an off-by-one error. Anyway, all this needs
> tests.

Fine with me.  It's a bit less elegant IMO, but you are right.  I had
to do it slightly differently since the line number needs to appear
irrespective of whether lines are included in the call initially.
That being said, I could have very well overlooked some obvioues way
of doing it.



Attachment: 0001-ox-Allow-file-links-with-INCLUDE-keyword.patch
Description: Text Data

reply via email to

[Prev in Thread] Current Thread [Next in Thread]