From mboxrd@z Thu Jan 1 00:00:00 1970 From: Joe Hirn Subject: Re: org-export-latex-hyperref-options-format Date: Thu, 20 Feb 2014 15:52:03 -0600 Message-ID: References: <87a9dpo0as.fsf@gmail.com> <8738jhnx1m.fsf@gmail.com> <878ut72qyo.fsf@gmail.com> <87zjlm13st.fsf@gmail.com> <8672745997102408713@unknownmsgid> <87vbw91pac.fsf@gmail.com> Mime-Version: 1.0 Content-Type: multipart/alternative; boundary=001a11c2ec9a16b20a04f2dd8422 Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:39588) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WGbXK-0008NK-Vu for emacs-orgmode@gnu.org; Thu, 20 Feb 2014 16:52:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WGbXJ-0000PL-QQ for emacs-orgmode@gnu.org; Thu, 20 Feb 2014 16:52:06 -0500 Received: from mail-ob0-x22f.google.com ([2607:f8b0:4003:c01::22f]:44905) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WGbXJ-0000Ot-Iz for emacs-orgmode@gnu.org; Thu, 20 Feb 2014 16:52:05 -0500 Received: by mail-ob0-f175.google.com with SMTP id wn1so2812501obc.34 for ; Thu, 20 Feb 2014 13:52:04 -0800 (PST) In-Reply-To: <87vbw91pac.fsf@gmail.com> List-Id: "General discussions about Org-mode." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org Sender: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org To: Nicolas Goaziou Cc: "emacs-orgmode@gnu.org" --001a11c2ec9a16b20a04f2dd8422 Content-Type: text/plain; charset=ISO-8859-1 I really appreciate your review. Sorry for the pedestrian code submission. I don't get candid critical feedback on my e-lisp, so it's great to learn more about its idioms and conventions. I'll incorporate your feedback into a new patch. On Thu, Feb 20, 2014 at 2:51 PM, Nicolas Goaziou wrote: > Hello, > > Joe Hirn writes: > > > I was able to test this on my local machine and it seems to work as we > > discussed. > > > > If there are any other changes to the patch you'd like to see, please let > > me know. > > Thank you for the patch. Here are a few comments. > > > - (:latex-hyperref-p nil "texht" org-latex-with-hyperref t) > > + (:latex-hyperref-p nil "texht" (if org-latex-hyperref-template t) t) > > I think we can drop the "-p" suffix since this is no longer a predicate. > So the property can be named :latex-hyperref. > > Also we can replace "texht" with nil since it doesn't make much sense to > specify a full template from the OPTIONS line. > > Eventually, the default value should be `org-latex-hyperref-template'. > > This boils down to the following line: > > (:latex-hyperref nil nil org-latex-hyperref-template t) > > > -(defcustom org-latex-with-hyperref t > > - "Toggle insertion of \\hypersetup{...} in the preamble." > > + > > +(defcustom org-latex-hyperref-template "\\hypersetup{\n > > pdfkeywords={%k},\n pdfsubject={%d},\n pdfcreator={%c}}\n" > > + "The value of \\hyperrefsetup{...} in the preamble. String is a > > format-spec which accepts keywords for %k (pdfkeywords), %d > > (pdfdescription) and %c (pdfcreator). Set to nil for no \\hyperrefsetup." > > :group 'org-export-latex > > - :type 'boolean) > > + :type 'string) > > The first line of the docstring should contain complete sentences only. > I would say something along the lines: > > "Template for hyperref package options. > > Value is a format string, which can contain the following placeholders: > > %k for KEYWORDS line > %d for DESCRIPTION line > %c for CREATOR line > > An empty string disables the setup." > > Since you specify :type as 'string, it is wrong to expect a nil value in > the variable. Note that nil is not an absolute necessity. We can allow > to disable the template with an empty string instead. > > > + (format-spec org-latex-hyperref-template > > + (format-spec-make > > + ?k (or (plist-get info :keywords) "") > > + ?d (or (plist-get info :description)"") > > + ?c (if (plist-get info :with-creator) > > + (plist-get info :creator) > > + "")))) > > You are not using the :latex-hyperref property. This should be: > > (format-spec (plist-get info :latex-hyperref) > ...) > > > Regards, > > -- > Nicolas Goaziou > --001a11c2ec9a16b20a04f2dd8422 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable
I really appreciate your review. Sorry for the pedestrian = code submission. I don't get candid critical feedback on my e-lisp, so = it's great to learn more about its idioms and conventions.=A0

<= /div>
I'll incorporate your feedback into a new patch.=A0


On Thu, Feb 20, = 2014 at 2:51 PM, Nicolas Goaziou <n.goaziou@gmail.com> wro= te:
Hello,

Joe Hirn <joseph.hirn@gmail.com= > writes:

> I was able to test this on my local machine and = it seems to work as we
> discussed.
>
> If there are any other changes to the patch you'd like to see, ple= ase let
> me know.

Thank you for the patch. Here are a few comments.

> - =A0 (:latex-hyperref-p nil "texht" org-latex-with-hyperref= t)
> + =A0 (:latex-hyperref-p nil "texht" (if org-latex-hyperref-= template t) t)

I think we can drop the "-p" suffix since this is no longer= a predicate.
So the property can be named :latex-hyperref.

Also we can replace "texht" with nil since it doesn't make mu= ch sense to
specify a full template from the OPTIONS line.

Eventually, the default value should be `org-latex-hyperref-template'.<= br>
This boils down to the following line:

=A0 (:latex-hyperref nil nil org-latex-hyperref-template t)

> -(defcustom org-latex-with-hyperref t
> - =A0"Toggle insertion of \\hypersetup{...} in the preamble."= ;
> +
> +(defcustom org-latex-hyperref-template "\\hypersetup{\n
> =A0pdfkeywords=3D{%k},\n =A0pdfsubject=3D{%d},\n =A0pdfcreator=3D{%c}}= \n"
> + =A0"The value of \\hyperrefsetup{...} in the preamble. String i= s a
> format-spec which accepts keywords for %k (pdfkeywords), %d
> (pdfdescription) and %c (pdfcreator). Set to nil for no \\hyperrefsetu= p."
> =A0 =A0:group 'org-export-latex
> - =A0:type 'boolean)
> + =A0:type 'string)

The first line of the docstring should contain complete sentences onl= y.
I would say something along the lines:

=A0 "Template for hyperref package options.

Value is a format string, which can contain the following placeholders:

=A0 %k for KEYWORDS line
=A0 %d for DESCRIPTION line
=A0 %c for CREATOR line

An empty string disables the setup."

Since you specify :type as 'string, it is wrong to expect a nil value i= n
the variable. Note that nil is not an absolute necessity. We can allow
to disable the template with an empty string instead.

> + =A0 =A0 =A0 (format-spec org-latex-hyperref-template
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(format-spec-make
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ?k (or (plist-get info :keyw= ords) "")
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ?d (or (plist-get info :desc= ription)"")
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ?c (if (plist-get info :with= -creator)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(plist-get in= fo :creator)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0""))))<= br>
You are not using the :latex-hyperref property. This should be:

=A0 (format-spec (plist-get info :latex-hyperref)
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0...)


Regards,

--
Nicolas Goaziou

--001a11c2ec9a16b20a04f2dd8422--