[RFC] [PATCH] Warn about unexpanded macros on export

[RFC] [PATCH] Warn about unexpanded macros on export

[Aaron Ecay]

[-- Attachment #1: Type: text/plain, Size: 1237 bytes --]

Hello all,

Currently, if a macro is not defined, it will silently produce an empty
string while exporting.  This situation could arise for example if a
macro name is acidentally mistyped.  I think it’s desirable to warn the
user in this case.  The attached patch introduces a function to do so,
and shows how it would be integrated in the latex backend.  This raises
several questions:

1. Should the warning be a “message” (allows the export process to
   continue) or a “user-error” (stops the export process)?  Or, should
   this be configurable?

2. Since this is a feature that many backends will want to use, should
   we introduce a new “abstract” backend from which other backends can
   inherit, which incorporates this feature, and others like it in the
   future?  The idea would be similar to prog-mode in emacs, the major
   mode from which programming-language modes can derive.  The
   alternative is adding the (macro . org-export-macro-warn) entry
   manually to all the relevant backends, and relying on future backend
   authors to do the same.

3. Should this even be implemented as part of the backend’s
   translate-alist, or at a lower level?

Thanks,

-- 
Aaron Ecay

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-ox-warn-if-unexpanded-macros-are-found-when-exportin.patch --]
[-- Type: text/x-diff, Size: 1324 bytes --]

From 1c9f85bcb93dbc56d01b138f5a4a11ad0933b5c6 Mon Sep 17 00:00:00 2001
From: Aaron Ecay <aaronecay@gmail.com>
Date: Fri, 19 Sep 2014 14:46:37 -0400
Subject: [PATCH] ox: warn if unexpanded macros are found when exporting

* lisp/ox.el (org-export-macro-warn): New function.
* lisp/ox-latex.el: Use it.
---
 lisp/ox-latex.el | 1 +
 lisp/ox.el       | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/lisp/ox-latex.el b/lisp/ox-latex.el
index f59d6b2..7670ccb 100644
--- a/lisp/ox-latex.el
+++ b/lisp/ox-latex.el
@@ -65,6 +65,7 @@
     (latex-fragment . org-latex-latex-fragment)
     (line-break . org-latex-line-break)
     (link . org-latex-link)
+    (macro . org-export-macro-warn)
     (node-property . org-latex-node-property)
     (paragraph . org-latex-paragraph)
     (plain-list . org-latex-plain-list)
diff --git a/lisp/ox.el b/lisp/ox.el
index f01f951..a4988f4 100644
--- a/lisp/ox.el
+++ b/lisp/ox.el
@@ -5619,6 +5619,11 @@ to `:default' encoding. If it fails, return S."
 	(plist-get translations :default)
 	s)))
 
+(defun org-export-macro-warn (macro contents info)
+  ;; TODO: should this be a user-error?
+  (message "WARNING: undefined macro %s" (org-element-property :key macro))
+  ;; Return empty string to avoid interfering with the export output.
+  "")
 
 \f
 ;;; Asynchronous Export
-- 
2.1.0

Re: [RFC] [PATCH] Warn about unexpanded macros on export

[Nicolas Goaziou]

Hello,

Aaron Ecay <aaronecay@gmail.com> writes:

> Currently, if a macro is not defined, it will silently produce an empty
> string while exporting.  This situation could arise for example if a
> macro name is acidentally mistyped.  I think it’s desirable to warn the
> user in this case.  The attached patch introduces a function to do so,
> and shows how it would be integrated in the latex backend.  This raises
> several questions:
>
> 1. Should the warning be a “message” (allows the export process to
>    continue) or a “user-error” (stops the export process)?  Or, should
>    this be configurable?

Certainly not a message, due to asynchronous export.

> 2. Since this is a feature that many backends will want to use, should
>    we introduce a new “abstract” backend from which other backends can
>    inherit, which incorporates this feature, and others like it in the
>    future?  The idea would be similar to prog-mode in emacs, the major
>    mode from which programming-language modes can derive.  The
>    alternative is adding the (macro . org-export-macro-warn) entry
>    manually to all the relevant backends, and relying on future backend
>    authors to do the same.
>
> 3. Should this even be implemented as part of the backend’s
>    translate-alist, or at a lower level?

Don't bother with export back-ends, they never get to see macros, which
are expanded very early in the export process. This explains, for
example, why there is no `macro' translator.

You have two options. Either report an error, as you suggested, or
insert an obnoxious message in the output, e.g., "UNKNOWN MACRO", à la
"DEFINITION NOT FOUND." for footnote definitions. In any case, this
should happen in "org-macro.el", not in the export framework.


Regards,

-- 
Nicolas Goaziou

Re: [RFC] [PATCH] Warn about unexpanded macros on export

[Aaron Ecay]

Hi Nicolas,

2014ko irailak 19an, Nicolas Goaziou-ek idatzi zuen:
> 
> Certainly not a message, due to asynchronous export.

Very good point.

> 
>> 2. Since this is a feature that many backends will want to use, should
>> we introduce a new “abstract” backend from which other backends can
>> inherit, which incorporates this feature, and others like it in the
>> future?  The idea would be similar to prog-mode in emacs, the major
>> mode from which programming-language modes can derive.  The
>> alternative is adding the (macro . org-export-macro-warn) entry
>> manually to all the relevant backends, and relying on future backend
>> authors to do the same.
>> 
>> 3. Should this even be implemented as part of the backend’s
>> translate-alist, or at a lower level?
> 
> Don't bother with export back-ends, they never get to see macros, which
> are expanded very early in the export process. This explains, for
> example, why there is no `macro' translator.

Um...but the patch I sent works precisely by defining a macro translator,
which does get called for any unexpanded (because undefined) macros.  I
guess you’re saying the code ought to block this at a lower/earlier level.

> 
> You have two options. Either report an error, as you suggested, or
> insert an obnoxious message in the output, e.g., "UNKNOWN MACRO", à la
> "DEFINITION NOT FOUND." for footnote definitions. In any case, this
> should happen in "org-macro.el", not in the export framework.

I think error is better than obnoxious message, because it’s possible
for the latter to slip through into a “production” document.  (We ought
to proofread our documents carefully, of course...but no one’s perfect).

One issue is that the exporter does two macro expansion passes – one
for garden-variety macros, and the second for author, date, email, and
title.  So, we can’t make the macro expansion unconditionally barf on
undefined macros (since for the first pass e.g. author is undefined).
I see three options:
1. explicitly whitelist the few “blessed” macros like author, and error
   on any other undefined macro
2. add an optional “final” arg to org-macro-replace-all, which will
   activate the undefined checking only if non-nil, and pass this flag
   in the exporter’s second (and last) call to org-macro-replace-all
3. in ‘org-export-as’, manually walk the parse tree after expanding
   macros, and make sure no 'macro type objects are left

WDYT?

-- 
Aaron Ecay

Re: [RFC] [PATCH] Warn about unexpanded macros on export

[Grant Rettke]

Aaron may I trouble you to add a flag so that if such errors occur
then the export fails?

From my perspective, if the document doesn't "compile", then nothing
should succeed.

Compile includes export from my perspective.

On Mon, Sep 22, 2014 at 10:25 PM, Aaron Ecay <aaronecay@gmail.com> wrote:
> Hi Nicolas,
>
> 2014ko irailak 19an, Nicolas Goaziou-ek idatzi zuen:
>>
>> Certainly not a message, due to asynchronous export.
>
> Very good point.
>
>>
>>> 2. Since this is a feature that many backends will want to use, should
>>> we introduce a new “abstract” backend from which other backends can
>>> inherit, which incorporates this feature, and others like it in the
>>> future?  The idea would be similar to prog-mode in emacs, the major
>>> mode from which programming-language modes can derive.  The
>>> alternative is adding the (macro . org-export-macro-warn) entry
>>> manually to all the relevant backends, and relying on future backend
>>> authors to do the same.
>>>
>>> 3. Should this even be implemented as part of the backend’s
>>> translate-alist, or at a lower level?
>>
>> Don't bother with export back-ends, they never get to see macros, which
>> are expanded very early in the export process. This explains, for
>> example, why there is no `macro' translator.
>
> Um...but the patch I sent works precisely by defining a macro translator,
> which does get called for any unexpanded (because undefined) macros.  I
> guess you’re saying the code ought to block this at a lower/earlier level.
>
>>
>> You have two options. Either report an error, as you suggested, or
>> insert an obnoxious message in the output, e.g., "UNKNOWN MACRO", à la
>> "DEFINITION NOT FOUND." for footnote definitions. In any case, this
>> should happen in "org-macro.el", not in the export framework.
>
> I think error is better than obnoxious message, because it’s possible
> for the latter to slip through into a “production” document.  (We ought
> to proofread our documents carefully, of course...but no one’s perfect).
>
> One issue is that the exporter does two macro expansion passes – one
> for garden-variety macros, and the second for author, date, email, and
> title.  So, we can’t make the macro expansion unconditionally barf on
> undefined macros (since for the first pass e.g. author is undefined).
> I see three options:
> 1. explicitly whitelist the few “blessed” macros like author, and error
>    on any other undefined macro
> 2. add an optional “final” arg to org-macro-replace-all, which will
>    activate the undefined checking only if non-nil, and pass this flag
>    in the exporter’s second (and last) call to org-macro-replace-all
> 3. in ‘org-export-as’, manually walk the parse tree after expanding
>    macros, and make sure no 'macro type objects are left
>
> WDYT?
>
> --
> Aaron Ecay
>



-- 
Grant Rettke
gcr@wisdomandwonder.com | http://www.wisdomandwonder.com/
“Wisdom begins in wonder.” --Socrates
((λ (x) (x x)) (λ (x) (x x)))
“Life has become immeasurably better since I have been forced to stop
taking it seriously.” --Thompson

Re: [RFC] [PATCH] Warn about unexpanded macros on export

[Aaron Ecay]

Hi Grant,

2014ko irailak 23an, Grant Rettke-ek idatzi zuen:
> 
> Aaron may I trouble you to add a flag so that if such errors occur
> then the export fails?
> 
> From my perspective, if the document doesn't "compile", then nothing
> should succeed.
> 
> Compile includes export from my perspective.

Indeed, that’s the direction that the next iteration of this patch will
move, motivated by your and Nicolas’s comments.

Thanks,

-- 
Aaron Ecay

Re: [RFC] [PATCH] Warn about unexpanded macros on export

[Jacob Gerlach]

[-- Attachment #1: Type: text/plain, Size: 1066 bytes --]

While I expect that there's no code overlap with macro expansion, I asked
about similar "stop on unresolved content" behavior with respect to links
here: http://article.gmane.org/gmane.emacs.orgmode/90010/

That discussion died off so I thought I'd bring up the topic again here.

Would it be possible to cause export to stop (configurably, as discussed)
when a link can't be resolved instead of just applying a special format
(i.e. org-latex-link-with-unknown-path-format)

On Tue, Sep 23, 2014 at 1:18 PM, Aaron Ecay <aaronecay@gmail.com> wrote:

> Hi Grant,
>
> 2014ko irailak 23an, Grant Rettke-ek idatzi zuen:
> >
> > Aaron may I trouble you to add a flag so that if such errors occur
> > then the export fails?
> >
> > From my perspective, if the document doesn't "compile", then nothing
> > should succeed.
> >
> > Compile includes export from my perspective.
>
> Indeed, that’s the direction that the next iteration of this patch will
> move, motivated by your and Nicolas’s comments.
>
> Thanks,
>
> --
> Aaron Ecay
>
>

[-- Attachment #2: Type: text/html, Size: 1645 bytes --]

Re: [RFC] [PATCH] Warn about unexpanded macros on export

[Nicolas Goaziou]

Hello,

Aaron Ecay <aaronecay@gmail.com> writes:

> Um...but the patch I sent works precisely by defining a macro translator,
> which does get called for any unexpanded (because undefined) macros.

Macros are not expected to be seen by export back-ends. It happens when
a macro is undefined, but this is not a reliable feature.

> I guess you’re saying the code ought to block this at a lower/earlier
> level.

Yes, at "org-macro.el" level.

> I think error is better than obnoxious message, because it’s possible
> for the latter to slip through into a “production” document.  (We ought
> to proofread our documents carefully, of course...but no one’s
> perfect).

Sounds good.

> One issue is that the exporter does two macro expansion passes – one
> for garden-variety macros, and the second for author, date, email, and
> title.  So, we can’t make the macro expansion unconditionally barf on
> undefined macros (since for the first pass e.g. author is undefined).
> I see three options:
> 1. explicitly whitelist the few “blessed” macros like author, and error
>    on any other undefined macro
> 2. add an optional “final” arg to org-macro-replace-all, which will
>    activate the undefined checking only if non-nil, and pass this flag
>    in the exporter’s second (and last) call to org-macro-replace-all
> 3. in ‘org-export-as’, manually walk the parse tree after expanding
>    macros, and make sure no 'macro type objects are left
>
> WDYT?

I have no strong opinion here but I lean towards option 2 as the error
stays internal to "org-macro.el" and is only triggered with an optional
argument. It also doesn't require to hardcode special macro names.


Regards,

-- 
Nicolas Goaziou

Re: [RFC] [PATCH] Warn about unexpanded macros on export

[Rasmus]

Hi, 

Aaron Ecay <aaronecay@gmail.com> writes:

>> You have two options. Either report an error, as you suggested, or
>> insert an obnoxious message in the output, e.g., "UNKNOWN MACRO", à la
>> "DEFINITION NOT FOUND." for footnote definitions. In any case, this
>> should happen in "org-macro.el", not in the export framework.

This is pretty annoying for footnotes.

> I think error is better than obnoxious message, because it’s possible
> for the latter to slip through into a “production” document.  (We ought
> to proofread our documents carefully, of course...but no one’s perfect).

I think an error is correct, but the message should be informative,
e.g. "macro MACRO undefined at LINE".

—Rasmus

-- 
Summon the Mothership!

Re: [RFC] [PATCH] Warn about unexpanded macros on export

[Nicolas Goaziou]

Hello,

Jacob Gerlach <jacobgerlach@gmail.com> writes:

> While I expect that there's no code overlap with macro expansion, I asked
> about similar "stop on unresolved content" behavior with respect to links
> here: http://article.gmane.org/gmane.emacs.orgmode/90010/
>
> That discussion died off so I thought I'd bring up the topic again here.
>
> Would it be possible to cause export to stop (configurably, as discussed)
> when a link can't be resolved instead of just applying a special format
> (i.e. org-latex-link-with-unknown-path-format)

I have no objection to return an error when a fuzzy link, and id link or
a custom id link cannot be matched.

However, I do not like the idea of configurability as it would make the
code a bit more complex for little gain.


Regards,

-- 
Nicolas Goaziou

Re: [RFC] [PATCH] Warn about unexpanded macros on export

[Aaron Ecay]

[-- Attachment #1: Type: text/plain, Size: 1918 bytes --]

Hi Nicolas,

Thanks for the feedback.

2014ko irailak 23an, Nicolas Goaziou-ek idatzi zuen:
> 
> Hello,
> 
> Aaron Ecay <aaronecay@gmail.com> writes:
> 
>> Um...but the patch I sent works precisely by defining a macro translator,
>> which does get called for any unexpanded (because undefined) macros.
> 
> Macros are not expected to be seen by export back-ends. It happens when
> a macro is undefined, but this is not a reliable feature.
> 
>> I guess you’re saying the code ought to block this at a lower/earlier
>> level.
> 
> Yes, at "org-macro.el" level.
> 
>> I think error is better than obnoxious message, because it’s possible
>> for the latter to slip through into a “production” document.  (We ought
>> to proofread our documents carefully, of course...but no one’s
>> perfect).
> 
> Sounds good.
> 
>> One issue is that the exporter does two macro expansion passes – one
>> for garden-variety macros, and the second for author, date, email, and
>> title.  So, we can’t make the macro expansion unconditionally barf on
>> undefined macros (since for the first pass e.g. author is undefined).
>> I see three options:
>> 1. explicitly whitelist the few “blessed” macros like author, and error
>> on any other undefined macro
>> 2. add an optional “final” arg to org-macro-replace-all, which will
>> activate the undefined checking only if non-nil, and pass this flag
>> in the exporter’s second (and last) call to org-macro-replace-all
>> 3. in ‘org-export-as’, manually walk the parse tree after expanding
>> macros, and make sure no 'macro type objects are left
>> 
>> WDYT?
> 
> I have no strong opinion here but I lean towards option 2 as the error
> stays internal to "org-macro.el" and is only triggered with an optional
> argument. It also doesn't require to hardcode special macro names.

Attached is a revised patch.  WDYT?

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Warn-about-unexpanded-macros-on-export.patch --]
[-- Type: text/x-diff, Size: 2983 bytes --]

From e386809601d99291885136de1d9be84193b69a2c Mon Sep 17 00:00:00 2001
From: Aaron Ecay <aaronecay@gmail.com>
Date: Sat, 27 Sep 2014 23:50:23 -0400
Subject: [PATCH] Warn about unexpanded macros on export

* lisp/org-macro.el (org-macro-replace-all): Add optional `finalize'
argument.
* lisp/ox.el (org-export-as): Use it.
---
 lisp/org-macro.el | 35 ++++++++++++++++++++++-------------
 lisp/ox.el        |  3 ++-
 2 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/lisp/org-macro.el b/lisp/org-macro.el
index 50ce438..307e232 100644
--- a/lisp/org-macro.el
+++ b/lisp/org-macro.el
@@ -155,10 +155,14 @@ default value.  Return nil if no template was found."
         ;; Return string.
         (format "%s" (or value ""))))))
 
-(defun org-macro-replace-all (templates)
+(defun org-macro-replace-all (templates &optional finalize)
   "Replace all macros in current buffer by their expansion.
+
 TEMPLATES is an alist of templates used for expansion.  See
-`org-macro-templates' for a buffer-local default value."
+`org-macro-templates' for a buffer-local default value.
+
+If optional arg FINALIZE is non-nil, raise an error if a macro is
+found in the buffer with no definition in TEMPLATES."
   (save-excursion
     (goto-char (point-min))
     (let (record)
@@ -176,17 +180,22 @@ TEMPLATES is an alist of templates used for expansion.  See
 	      (if (member signature record)
 		  (error "Circular macro expansion: %s"
 			 (org-element-property :key object))
-		(when value
-		  (push signature record)
-		  (delete-region
-		   begin
-		   ;; Preserve white spaces after the macro.
-		   (progn (goto-char (org-element-property :end object))
-			  (skip-chars-backward " \t")
-			  (point)))
-		  ;; Leave point before replacement in case of recursive
-		  ;; expansions.
-		  (save-excursion (insert value)))))))))))
+		(if value
+		    (progn
+		      (push signature record)
+		      (delete-region
+		       begin
+		       ;; Preserve white spaces after the macro.
+		       (progn (goto-char (org-element-property :end object))
+			      (skip-chars-backward " \t")
+			      (point)))
+		      ;; Leave point before replacement in case of recursive
+		      ;; expansions.
+		      (save-excursion (insert value)))
+		  (when finalize
+		    (error "Macro %s was undefined at line %s"
+			   (org-element-property :key object)
+			   (line-number-at-pos))))))))))))
 
 
 (provide 'org-macro)
diff --git a/lisp/ox.el b/lisp/ox.el
index 59091fc..216a375 100644
--- a/lisp/ox.el
+++ b/lisp/ox.el
@@ -3136,7 +3136,8 @@ Return code as a string."
 		;; EMAIL is not a parsed keyword: store it as-is.
 		(cons "email" (or (plist-get info :email) ""))
 		(cons "title"
-		      (org-element-interpret-data (plist-get info :title)))))
+		      (org-element-interpret-data (plist-get info :title))))
+	  'finalize)
 	 ;; Parse buffer.
 	 (setq tree (org-element-parse-buffer nil visible-only))
 	 ;; Handle left-over uninterpreted elements or objects in
-- 
2.1.1


[-- Attachment #3: Type: text/plain, Size: 15 bytes --]

-- 
Aaron Ecay

Re: [RFC] [PATCH] Warn about unexpanded macros on export

[Aaron Ecay]

[-- Attachment #1: Type: text/plain, Size: 907 bytes --]

Hi Rasmus,

2014ko irailak 23an, Rasmus-ek idatzi zuen:
> 
> Hi, 
> 
> Aaron Ecay <aaronecay@gmail.com> writes:
> 
>>> You have two options. Either report an error, as you suggested, or
>>> insert an obnoxious message in the output, e.g., "UNKNOWN MACRO", à la
>>> "DEFINITION NOT FOUND." for footnote definitions. In any case, this
>>> should happen in "org-macro.el", not in the export framework.
> 
> This is pretty annoying for footnotes.

This turned out to be very easy to change; attached is a patch.  The
links issue (re-)raised by Jacob in
<http://mid.gmane.org/CAA6UvuFm-1nWD06A5o3HwSvEQmgqLJNcfK_PtoHSEhcVdJTYQw@mail.gmail.com>
is a bit harder to deal with, since each backend currently does
something a little different.  It would be possible to make every
backend’s org-X-link function error out at the end, but I’m not sure if
that’s the right thing to do or not.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0002-export-Raise-an-error-if-footnote-definition-is-not-.patch --]
[-- Type: text/x-diff, Size: 1285 bytes --]

From 123d5fcd4f4594c04210ab873395e7def8790450 Mon Sep 17 00:00:00 2001
From: Aaron Ecay <aaronecay@gmail.com>
Date: Sat, 27 Sep 2014 23:57:01 -0400
Subject: [PATCH 2/2] [export] Raise an error if footnote definition is not
 found.

* lisp/ox.el (org-export-get-footnote-definition): Raise an error if
footnote definition is not found.
---
 lisp/ox.el | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lisp/ox.el b/lisp/ox.el
index 216a375..2516a22 100644
--- a/lisp/ox.el
+++ b/lisp/ox.el
@@ -3711,12 +3711,12 @@ INFO is the plist used as a communication channel."
 (defun org-export-get-footnote-definition (footnote-reference info)
   "Return definition of FOOTNOTE-REFERENCE as parsed data.
 INFO is the plist used as a communication channel.  If no such
-definition can be found, return \"DEFINITION NOT FOUND\"."
+definition can be found, raise an error."
   (let ((label (org-element-property :label footnote-reference)))
     (or (if label
 	    (cdr (assoc label (plist-get info :footnote-definition-alist)))
 	  (org-element-contents footnote-reference))
-	"DEFINITION NOT FOUND.")))
+	(error "Definition not found for footnote %s" label))))
 
 (defun org-export-get-footnote-number (footnote info)
   "Return number associated to a footnote.
-- 
2.1.1


[-- Attachment #3: Type: text/plain, Size: 15 bytes --]

-- 
Aaron Ecay

Re: [RFC] [PATCH] Warn about unexpanded macros on export

[Nicolas Goaziou]

Hello,

Aaron Ecay <aaronecay@gmail.com> writes:

> Attached is a revised patch.  WDYT?

Looks good. Some small comments follow.

> +		(if value
> +		    (progn
> +		      (push signature record)
> +		      (delete-region
> +		       begin
> +		       ;; Preserve white spaces after the macro.
> +		       (progn (goto-char (org-element-property :end object))
> +			      (skip-chars-backward " \t")
> +			      (point)))
> +		      ;; Leave point before replacement in case of recursive
> +		      ;; expansions.
> +		      (save-excursion (insert value)))
> +		  (when finalize
> +		    (error "Macro %s was undefined at line %s"
> +			   (org-element-property :key object)
> +			   (line-number-at-pos))))))))))))

Nitpick: I find the following more readable

  (cond (value (push signature record)
               ...)
        (finalize (error ...)))

Also, don't provide error line as macro are replaced after include
keywords are expanded. IOW, in some cases, the line number will be
misleading. The key is sufficient, e.g.,

  (error "Undefined Org macro: %s.  Aborting"
         (org-element-property :key object))

You can commit it once this is fixed. Thank you for the patch.


Regards,

-- 
Nicolas Goaziou

Re: [RFC] [PATCH] Warn about unexpanded macros on export

[Nicolas Goaziou]

Hello,

Aaron Ecay <aaronecay@gmail.com> writes:

> This turned out to be very easy to change; attached is a patch.

Thank you. Please apply it.

> The links issue (re-)raised by Jacob in
> <http://mid.gmane.org/CAA6UvuFm-1nWD06A5o3HwSvEQmgqLJNcfK_PtoHSEhcVdJTYQw@mail.gmail.com>
> is a bit harder to deal with, since each backend currently does
> something a little different. It would be possible to make every
> backend’s org-X-link function error out at the end, but I’m not sure
> if that’s the right thing to do or not.

A more workable solution would be to focus on internal links only and
patch `org-export-resolve-id-link', `org-export-resolve-fuzzy-link',
`org-export-resolve-coderef' (not needed for radio links).


Regards,

-- 
Nicolas Goaziou

Re: [RFC] [PATCH] Warn about unexpanded macros on export

[Aaron Ecay]

Hi Nicolas,

2014ko irailak 28an, Nicolas Goaziou-ek idatzi zuen:
> 
> Hello,
> 
> Aaron Ecay <aaronecay@gmail.com> writes:
> 
>> Attached is a revised patch.  WDYT?
> 
> Looks good. Some small comments follow.
> 
>> +		(if value
>> +		    (progn
>> +		      (push signature record)
>> +		      (delete-region
>> +		       begin
>> +		       ;; Preserve white spaces after the macro.
>> +		       (progn (goto-char (org-element-property :end object))
>> +			      (skip-chars-backward " \t")
>> +			      (point)))
>> +		      ;; Leave point before replacement in case of recursive
>> +		      ;; expansions.
>> +		      (save-excursion (insert value)))
>> +		  (when finalize
>> +		    (error "Macro %s was undefined at line %s"
>> +			   (org-element-property :key object)
>> +			   (line-number-at-pos))))))))))))
> 
> Nitpick: I find the following more readable
> 
>   (cond (value (push signature record)
>                ...)
>         (finalize (error ...)))
> 
> Also, don't provide error line as macro are replaced after include
> keywords are expanded. IOW, in some cases, the line number will be
> misleading. The key is sufficient, e.g.,
> 
>   (error "Undefined Org macro: %s.  Aborting"
>          (org-element-property :key object))
> 
> You can commit it once this is fixed. Thank you for the patch.

Pushed; thanks for the feedback.

-- 
Aaron Ecay

Re: [RFC] [PATCH] Warn about unexpanded macros on export

[Aaron Ecay]

2014ko irailak 28an, Nicolas Goaziou-ek idatzi zuen:
> 
> Hello,
> 
> Aaron Ecay <aaronecay@gmail.com> writes:
> 
>> This turned out to be very easy to change; attached is a patch.
> 
> Thank you. Please apply it.

Pushed.

-- 
Aaron Ecay

Re: [RFC] [PATCH] Warn about unexpanded macros on export

[Nicolas Goaziou]

Hello,

Aaron Ecay <aaronecay@gmail.com> writes:

> Pushed.

Thanks.

-- 
Nicolas Goaziou                                                0x80A93738

Re: [RFC] [PATCH] Warn about unexpanded macros on export

[Jacob Gerlach]

[-- Attachment #1: Type: text/plain, Size: 2096 bytes --]

On Sun, Sep 28, 2014 at 3:03 AM, Nicolas Goaziou <mail@nicolasgoaziou.fr> wrote:
> Aaron Ecay <aaronecay@gmail.com> writes:
>> The links issue (re-)raised by Jacob in
>> <http://mid.gmane.org/CAA6UvuFm-1nWD06A5o3HwSvEQmgqLJNcfK_PtoHSEhcVdJTYQw@mail.gmail.com>
>> is a bit harder to deal with, since each backend currently does
>> something a little different. It would be possible to make every
>> backend’s org-X-link function error out at the end, but I’m not sure
>> if that’s the right thing to do or not.
>
> A more workable solution would be to focus on internal links only and
> patch `org-export-resolve-id-link', `org-export-resolve-fuzzy-link',
> `org-export-resolve-coderef' (not needed for radio links).

Patch attached for fuzzy links.

I don't really use code blocks, so I wasn't sure what to do with
org-export-resolve-coderef. Should the final

(when (re-search...
  (cond ...

become

(or (re-search...
   (cond ...
 (error

ID links are tricky. AFAICT, an invalid id link will always resolve to
the file it's contained in. The last check in
org-export-resolve-id-link is looking for the id in `:id-alist'. Is
this the same `:id-alist' built in org-export-get-environment?

If so, is it desirable for org-id-find-id-file to fall back on the
current buffer (the current behavior)? It seems like this will
mistakenly cause org-export-get-environment to think that the bad link
is valid and pointing to an "external" file. IIUC, the fall back
behavior doesn't occur inside org-test-with-parsed-data because
buffer-file-name returns nil.

I don't know the implications of changing
org-id-find-id-file. The fall back behavior was introduced in ac83bc01
when org-id was mostly rewritten. Removing the fall back behavior
doesn't cause any failures on `make test'. If it's acceptable to
remove the fall back, I can provide a similar patch for
org-export-resolve-id-link.

Example, foo.org:

[[id:points-nowhere]]

[[id:this-one-too][also bad]]

Exports (latex) to:

\url{foo.org}

\href{foo.org}{also bad}

Regards,
Jake

[-- Attachment #2: 0001-ox.el-Issue-error-for-unresolved-fuzzy-link.patch --]
[-- Type: text/x-patch, Size: 2255 bytes --]

From 20f84420a84997fc0e15df5af2e65b3cfde87ac1 Mon Sep 17 00:00:00 2001
From: Jacob Gerlach <jacobgerlach@gmail.com>
Date: Wed, 11 Mar 2015 22:39:11 -0400
Subject: [PATCH] ox.el: Issue error for unresolved fuzzy link

* lisp/org-capture.el (org-export-resolve-fuzzy-link): throw error
  instead of returning nil when link can't be resolved.

* testing/lisp/test-ox.el (test-org-export/resolve-fuzzy-link): change
  last test from should-not to should-error

In addition to throwing an error, don't store the failed match in the
link cache.

TINYCHANGE
---
 lisp/ox.el              | 7 ++++---
 testing/lisp/test-ox.el | 4 ++--
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/lisp/ox.el b/lisp/ox.el
index 0c7728f..a28a227 100644
--- a/lisp/ox.el
+++ b/lisp/ox.el
@@ -4068,7 +4068,7 @@ significant."
      ;; Last case: link either points to a headline or to nothingness.
      ;; Try to find the source, with priority given to headlines with
      ;; the closest common ancestor.  If such candidate is found,
-     ;; return it, otherwise return nil.
+     ;; return it, otherwise signal an error.
      (t
       (let ((find-headline
 	     (function
@@ -4094,8 +4094,9 @@ significant."
 		       (org-element-lineage parent-hl nil t))))
 	    (let ((foundp (funcall find-headline path parent)))
 	      (when foundp (throw 'exit foundp))))
-	  ;; No destination found: return nil.
-	  (and (not match-title-p) (puthash path nil link-cache))))))))
+	  ;; No destination found: error.
+	  (unless match-title-p
+	    (error (format "Unable to resolve link \"%s\"" raw-path)))))))))
 
 (defun org-export-resolve-id-link (link info)
   "Return headline referenced as LINK destination.
diff --git a/testing/lisp/test-ox.el b/testing/lisp/test-ox.el
index 1b70a78..7cf1e1d 100644
--- a/testing/lisp/test-ox.el
+++ b/testing/lisp/test-ox.el
@@ -2478,8 +2478,8 @@ Another text. (ref:text)
 	 (org-element-type
 	  (org-export-resolve-fuzzy-link
 	   (org-element-map tree 'link 'identity info t) info)))))
-  ;; Return nil if no match.
-  (should-not
+  ;; Error if no match.
+  (should-error
    (org-test-with-parsed-data "[[target]]"
      (org-export-resolve-fuzzy-link
       (org-element-map tree 'link 'identity info t) info)))
-- 
1.9.1

Re: [RFC] [PATCH] Warn about unexpanded macros on export

[Nicolas Goaziou]

Jacob Gerlach <jacobgerlach@gmail.com> writes:

> Patch attached for fuzzy links.

Thank you.

> I don't really use code blocks, so I wasn't sure what to do with
> org-export-resolve-coderef. Should the final
>
> (when (re-search...
>   (cond ...
>
> become
>
> (or (re-search...
>    (cond ...
>  (error

No, the whole body could be wrapped with an `or':

  (or (org-element-map ...)
      (user-error "Unable to resolve code reference: %s" ref))

> ID links are tricky. AFAICT, an invalid id link will always resolve to
> the file it's contained in. The last check in
> org-export-resolve-id-link is looking for the id in `:id-alist'. Is
> this the same `:id-alist' built in org-export-get-environment?

Yes, it is.

> If so, is it desirable for org-id-find-id-file to fall back on the
> current buffer (the current behavior)?

According to its docstring, `org-id-find-id-file' returns nil when
search failed. Isn't it the case?

> I don't know the implications of changing org-id-find-id-file. The
> fall back behavior was introduced in ac83bc01 when org-id was mostly
> rewritten. Removing the fall back behavior doesn't cause any failures
> on `make test'. If it's acceptable to remove the fall back, I can
> provide a similar patch for org-export-resolve-id-link.

`org-export-resolve-id-link' could throw an error, indeed.

> Subject: [PATCH] ox.el: Issue error for unresolved fuzzy link
>
> * lisp/org-capture.el (org-export-resolve-fuzzy-link): throw error
         ^^^^^^^^^^^
         wrong file

You need to capitalize sentence: "Throw an error"

>   instead of returning nil when link can't be resolved.
>
> * testing/lisp/test-ox.el (test-org-export/resolve-fuzzy-link): change
>   last test from should-not to should-error

Ditto.

> +	  ;; No destination found: error.
> +	  (unless match-title-p
> +	    (error (format "Unable to resolve link \"%s\"" raw-path)))))))))

You don't need to check `match-title-p' here. Also, it should be
`user-error' instead of `error'.


Regards,

Re: [RFC] [PATCH] Warn about unexpanded macros on export

[Jacob Gerlach]

[-- Attachment #1: Type: text/plain, Size: 1975 bytes --]

Hello,

Thanks for the feedback.

On Sun, Mar 15, 2015 at 4:44 AM, Nicolas Goaziou <mail@nicolasgoaziou.fr> wrote:
>> I don't really use code blocks, so I wasn't sure what to do with
>> org-export-resolve-coderef.
[...]
> No, the whole body could be wrapped with an `or':
>
>   (or (org-element-map ...)
>       (user-error "Unable to resolve code reference: %s" ref))
I tried this, or more specifically:
(or ((org-element-map
...
    info 'first-match))
  (user-error ...
 and got a failure on test-org-export/resolve-coderef. It's not
obvious to me from reading the tests if there is a test that needs to
be changed, or if it's a legitimate failure and a different approach
is needed (or if I made a mistake). I asked in [1] for some guidance
on tests, and I'm still lost.

>> If so, is it desirable for org-id-find-id-file to fall back on the
>> current buffer (the current behavior)?
>
> According to its docstring, `org-id-find-id-file' returns nil when
> search failed. Isn't it the case?
Are you looking at `org-id-find-id-in-file' rather than
`org-id-find-id-file'? The docstring for `org-id-find-file' only says:
"Query the id database for the file in which this ID is located."

[...]
> `org-export-resolve-id-link' could throw an error, indeed.
I'm not clear on the way forward for id links. I propose removing the
fall back behavior in `org-id-find-id-file'. If that's acceptable, I
can provide a patch for `org-export-resolve-id-link'.

>> Subject: [PATCH] ox.el: Issue error for unresolved fuzzy link
[...]
Commit message fixed. I also updated the docstring for
org-export-resolve-fuzzy link.
>> +       ;; No destination found: error.
>> +       (unless match-title-p
>> +         (error (format "Unable to resolve link \"%s\"" raw-path)))))))))
>
> You don't need to check `match-title-p' here. Also, it should be
> `user-error' instead of `error'.
OK.

Updated fuzzy link patch attached.

Regards,
Jake

[1] http://thread.gmane.org/gmane.emacs.orgmode/96017

[-- Attachment #2: 0001-ox.el-Issue-error-for-unresolved-fuzzy-link.patch --]
[-- Type: text/x-patch, Size: 2550 bytes --]

From a9221500b721a501f7b7f05519dfc0d6f30f23d0 Mon Sep 17 00:00:00 2001
From: Jacob Gerlach <jacobgerlach@gmail.com>
Date: Wed, 11 Mar 2015 22:39:11 -0400
Subject: [PATCH] ox.el: Issue error for unresolved fuzzy link

* lisp/ox.el (org-export-resolve-fuzzy-link): Throw an error instead
  of returning nil when link can't be resolved.

* testing/lisp/test-ox.el (test-org-export/resolve-fuzzy-link): Change
  last test from should-not to should-error

In addition to throwing an error, don't store the failed match in the
link cache.

TINYCHANGE
---
 lisp/ox.el              | 8 ++++----
 testing/lisp/test-ox.el | 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/lisp/ox.el b/lisp/ox.el
index 0c7728f..9461117 100644
--- a/lisp/ox.el
+++ b/lisp/ox.el
@@ -4021,7 +4021,7 @@ Return value can be an object, an element, or nil:
   will be given to the one with the closest common ancestor, if
   any, or the first one in the parse tree otherwise.
 
-- Otherwise, return nil.
+- Otherwise, report an error.
 
 Assume LINK type is \"fuzzy\".  White spaces are not
 significant."
@@ -4068,7 +4068,7 @@ significant."
      ;; Last case: link either points to a headline or to nothingness.
      ;; Try to find the source, with priority given to headlines with
      ;; the closest common ancestor.  If such candidate is found,
-     ;; return it, otherwise return nil.
+     ;; return it, otherwise signal an error.
      (t
       (let ((find-headline
 	     (function
@@ -4094,8 +4094,8 @@ significant."
 		       (org-element-lineage parent-hl nil t))))
 	    (let ((foundp (funcall find-headline path parent)))
 	      (when foundp (throw 'exit foundp))))
-	  ;; No destination found: return nil.
-	  (and (not match-title-p) (puthash path nil link-cache))))))))
+	  ;; No destination found: error.
+	    (user-error (format "Unable to resolve link \"%s\"" raw-path))))))))
 
 (defun org-export-resolve-id-link (link info)
   "Return headline referenced as LINK destination.
diff --git a/testing/lisp/test-ox.el b/testing/lisp/test-ox.el
index 1b70a78..7cf1e1d 100644
--- a/testing/lisp/test-ox.el
+++ b/testing/lisp/test-ox.el
@@ -2478,8 +2478,8 @@ Another text. (ref:text)
 	 (org-element-type
 	  (org-export-resolve-fuzzy-link
 	   (org-element-map tree 'link 'identity info t) info)))))
-  ;; Return nil if no match.
-  (should-not
+  ;; Error if no match.
+  (should-error
    (org-test-with-parsed-data "[[target]]"
      (org-export-resolve-fuzzy-link
       (org-element-map tree 'link 'identity info t) info)))
-- 
1.9.1

Re: [RFC] [PATCH] Warn about unexpanded macros on export

[Nicolas Goaziou]

Jacob Gerlach <jacobgerlach@gmail.com> writes:

> I tried this, or more specifically:
> (or ((org-element-map
> ...
>     info 'first-match))
>   (user-error ...
>  and got a failure on test-org-export/resolve-coderef. It's not
> obvious to me from reading the tests if there is a test that needs to
> be changed, or if it's a legitimate failure and a different approach
> is needed (or if I made a mistake). I asked in [1] for some guidance
> on tests, and I'm still lost.

There is a spurious (...) in the code above.

I implemented it in a886b234763f288670103d2c98169164dedec06c.

>> According to its docstring, `org-id-find-id-file' returns nil when
>> search failed. Isn't it the case?
> Are you looking at `org-id-find-id-in-file' rather than
> `org-id-find-id-file'? The docstring for `org-id-find-file' only says:
> "Query the id database for the file in which this ID is located."

You're right.

>> `org-export-resolve-id-link' could throw an error, indeed.
> I'm not clear on the way forward for id links. I propose removing the
> fall back behavior in `org-id-find-id-file'. If that's acceptable, I
> can provide a patch for `org-export-resolve-id-link'.

Actually, we don't even need to do that. I changed "ox.el" so it uses
`org-id-find' instead of `org-id-find-id-file'. The former has a better
fall-back value. See

  e0b19dedb99d3c01199a159a2da8f9aa4adb2f6f

> Updated fuzzy link patch attached.

Applied (with minor tweaks). Thank you.


Regards,

Re: [RFC] [PATCH] Warn about unexpanded macros on export

[Jacob Gerlach]

On Tue, Mar 17, 2015 at 6:38 PM, Nicolas Goaziou <mail@nicolasgoaziou.fr> wrote:
(re unresolved ID links)
>See e0b19dedb99d3c01199a159a2da8f9aa4adb2f6f

>> Updated fuzzy link patch attached.
> Applied (with minor tweaks). Thank you.

Thanks for the changes.

I had some issues using the error messages to track down bad links
with descriptions since the link path is hidden. (isearch doesn't
search the hidden text.) If anyone else is similarly challenged,
`org-toggle-link-display' is very helpful here.

Regards,
Jake