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

[Nicolas Goaziou <mail@nicolasgoaziou.fr>]

Hello,

Rasmus <rasmus@gmx.us> writes:

> Okay, I hope I got a better patch here.

Thank you. Some comments follow.

> Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:
>> This should be ":only-contents t" or ":only-contents nil".
>> ":only-contents" alone can be tolerated as a shortcut for
>> ":only-contents nil", but that's all.
>
> 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.

> It doesn't make sense to apply this to source files, right?

Right.

> +elements.}.  If the keyword @code{:only-contents} is used, only the contents
> +of the included element.

The sentence is not complete. Also, it should be something like "If you
set @code{:only-contents} property to a non-nil value, only...".

> For headlines, drawers and properties
> +immediately following the headline will not be included when using
> +@code{:only-contents}.

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).

>  Some examples:
> +
> +@example
> +#+INCLUDE: "./paper.org::#theory" :only-contents

  #+INCLUDE: "./paper.org::#theory" :only-contents t

> +@smallexample
> +#+INCLUDE: "./otherfile.org::#my_custom_id" :no-contents
> +@end smallexample

  #+INCLUDE: "./otherfile.org::#my_custom_id" :only-contents t

> +			    (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)))

> +		 (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))))

> +		   (let ((org-inhibit-startup t)
> +			 (lines (org-export--inclusion-absolute-lines
> +				 file  location only-contents lines)))

Lines are local to the element only if LOCATION is provided, right?

  (lines (if location
             (org-export--inclusion-absolute-lines ...)
           lines))

> +  (with-temp-buffer
> +    (insert-file-contents file)
> +    (when location

This check is useless since we know location is defined (otherwise,
this function wouldn't be called).

> +      ;; locations are only defined for org files so
> +      ;; OK to start org-mode.

You can remove this.

> +      (condition-case err
> +	  ;; enforce consistency in search.

"Enforce"

> +	  (let ((org-link-search-must-match-exact-headline t))
> +	    (org-link-search location))
> +	;; helpful error messages

You can remove this.

> +	(error
> +	 (error (format "%s for %s::%s" (error-message-string err) file location))))
> +      (org-mode)

Shouldn't (org-mode) be moved before calling `org-link-search'? At some
point, `org-link-search' may use Elements.

> +      (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))))

> +    (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'.

> +	(narrow-to-region beg end)))

This is not needed.

> +    (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.

> +   (switch-to-buffer-other-window (current-buffer))
> +   (insert-file-contents file)

Why is it needed?

>      (when ind
>        (unless (eq major-mode 'org-mode)
>  	(let ((org-inhibit-startup t)) (org-mode)))
> +      ;; make sure that there is not an immediate "lonely"
> +      ;; property-drawer.  Normal drawers are OK but property-drawers
> +      ;; may follow normal drawers.
> +      (goto-char (point-min))
> +      (let ((element (org-element-at-point)))
> +	(while (memq (org-element-type element) '(drawer property-drawer))
> +	  ;; entering here the first element is not a heading so it's
> +	  ;; safe to get rid of property-drawers.
> +	  (if (eq (org-element-type element) 'property-drawer)
> +	      (delete-region (org-element-property :begin element)
> +			     (org-element-property :end element))
> +	  (goto-char (org-element-property :end element)))
> +	  (setq element (org-element-at-point))))

Why do you need to skip anything since function above already took care
of that?


Regards,

-- 
Nicolas Goaziou