Re: [ox-publish, patch] More flexible sitemaps

[Nicolas Goaziou <mail@nicolasgoaziou.fr>]

Hello,

Rasmus <rasmus@gmx.us> writes:

> This was by far the hardest part...

Thank you. Some comments follow.

> +(defun org-publish-find-property (file property &optional reset)
> +  "Find the PROPERTY of FILE in project.
> +PROPERTY can be a string or a symbol-property."

Could you also document RESET argument?

> +  (unless (or (file-directory-p file) (directory-name-p file))

What is `directory-name-p'?

> +    (let ((prop (cond ((stringp property)
> +                       (intern (downcase (format ":%s" property))))
> +                      (t property))))

The following might be more robust:

  (if (keywordp property) property
    (intern (downcase (format ":%s" property)))

> +      (and (not reset) (org-publish-cache-get-file-property file prop nil t))

(unless reset ...)

Anyway, you don't seem to use the return value. If this is used for
side-effect only, you may want to call `org-publish-initialize-cache'
instead.

> +      (let* ((org-inhibit-startup t)
> +             (visiting (find-buffer-visiting file))
> +             (buffer (or visiting (find-file-noselect file)))
> +             (case-fold-search t))
> +        (with-current-buffer buffer
> +          ;; protect local variables in open buffers
> +          (org-export-with-buffer-copy
> +           (let* ((backends (append (list nil)
> +                                    (mapcar 'org-export-backend-name
> +                                            org-export-registered-backends)))
> +                  (value (cl-loop for backend in backends
> +                                  when (org-no-properties
> +                                        (org-element-interpret-data
> +                                         (plist-get (org-export-get-environment backend)
> +                                                    prop)))
> +                                  return it)))

Return value of `org-element-interpret-data' is never nil so the loop
always returns the first back-end.

In any case, this is sub-optimal since common keywords are retrieved for
each back-end. Also, two back-ends could use the same keyword, with
different values (e.g., using `parsed' or not).

One option could be to change the policy about keywords in ox.el and
move KEYWORDS and SUBTITLE there. Unfortunately, there is still a change
to miss some cases.

Another option would be to provide BACKEND to sitemap-function. I think
it can be backward-compatible if we make it an optional argument.

> +(defcustom org-publish-sitemap-file-entry-format "%- [[file:%link][%TITLE]]"
>    "Format string for site-map file entry.
> -You could use brackets to delimit on what part the link will be.
>  
> -%t is the title.
> -%a is the author.
> -%d is the date formatted using `org-publish-sitemap-date-format'."
> +Most keywords can be accessed using a the name of the keyword
> +prefixed with \"%\", e.g. \"%TITLE\" or \"%KEYWORDS\".  In
> +addition, the following keywords are defined.
> +
> +  %fullpath
> +    The full path of the file.
> +
> +  %relpath
> +    The relative path of the file.
> +
> +  %filename
> +  %f
> +    The filename.
> +
> +  %link
> +  %l
> +    The link.
> +
> +  %*
> +    Leveled headline relative to the base directory.
> +
> +  %*
> +    Leveled item relative to the base directory.
> +
> +  %author
> +    The author of the document, the project, or `user-full-name'.
> +
> +  %date
> +     Date formatted using `org-publish-sitemap-date-format'.
> +
> +See also `org-publish-sitemap-dir-entry-format'."

Could it be possible to use single-char placeholders and `format-spec',
which is more robust than replace-match (e.g., it is possibl to escape
percent signs)...

> +(defcustom org-publish-sitemap-dir-entry-format "%- %f"
> +  "Format string for site-map file entry.
> +
> +The following keywords are defined.
> +
> +  %fullpath
> +    The full path of the file.
> +
> +  %relpath
> +    The relative path of the file.
> +
> +  %filename
> +  %f
> +    The filename.
> +
> +  %link
> +  %l
> +    The link.
> +
> +  %*
> +    Leveled headline relative to the base directory.
> +
> +  %*
> +    Leveled item relative to the base directory.
> +
> +  %author
> +    The author of the document, the project, or `user-full-name'.
> +
> +  %date
> +     Date formatted using `org-publish-sitemap-date-format'.

Ditto.

> @@ -1292,11 +1394,11 @@ Returns value on success, else nil."
>  (defun org-publish-cache-ctime-of-src (file)
>    "Get the ctime of FILE as an integer."
>    (let ((attr (file-attributes
> -	       (expand-file-name (or (file-symlink-p file) file)
> -				 (file-name-directory file)))))
> +               (expand-file-name (or (file-symlink-p file) file)
> +                                 (file-name-directory file)))))

Maybe 

  (file-truename file)

instead.

> +  `:sitemap-preamble'
> +  `:sitemap-postamble'
> +
> +    Preamble and postamble for sitemap.  Useful for inserting
> +    #+OPTIONS: keywords, footers etc.  See
> +    `org-publish-sitemap-preamble' for details.

Note there are not much details in `org-publish-sitemap-preamble' either.

> +(defcustom org-publish-sitemap-preamble nil
> +  "Sitemap preamble.
> +
> +Can be either a string, a list of strings, or a function that
> +takes a project-plist as an argument and return a string."

"returns"

Is allowing both strings and lists of strings useful enough?

> +Can be either a string, a list of strings, or a function that
> +takes a project-plist as an argument and return a string."

See above.

> +	 (sitemap-title (or (plist-get project-plist :sitemap-title)
> +			    (concat "Sitemap for project " (car project))))
> +	 (prepare-pre-or-postamble (lambda (pre-or-postamble)

I suggest to move lambda on the next line and use a less mouthful name
for the argument.

> +				     (cond ((not pre-or-postamble) nil)
> +					   ((stringp pre-or-postamble) pre-or-postamble)
> +					   ((listp pre-or-postamble)
> +					    (mapconcat 'identity preamble "\n"))

#'identity

> +					   ((functionp pre-or-postamble)
> +					    (funcall pre-or-postamble project-plist))
> +					   (t (error (concat "unknown `:sitemap-preamble' or "
> +							     "`:sitemap-postamble' format")))))))

I think `concat' is not necessary with the indentation rule suggested
above. Also: "Unknown".

> +	 ;; Call function to build sitemap based on files and the project-plist.
> +	 (let* ((style (or (plist-get project-plist :sitemap-style) 'tree))
> +		(fun (intern (format "org-publish-org-sitemap-as-%s" style))))

Side note : I think this is a bit too smart. It prevents, e.g., from
grepping for a function name. Maybe 

  (if (eq style 'something) #'... #'....)

would be better.

> +	   (unless (functionp fun)
> +	     (error "Unknown function %s for :sitemap-style %s."
> +		    fun style))

I know this is not directly related to your patch, but it should
probably be `user-error'. Also, error messages are not expected to end
on a period.

> +@item @code{:sitemap-preamble}
> +@item @code{:sitemap-postamble}
> +@tab Preamble and postamble for sitemap.  Useful for inserting
> +    @code{#+OPTIONS}, footers etc.  See @code{org-publish-sitemap-preamble}
> +    for details.

I don't understand the "useful for inserting @code{#+OPTIONS}" part.
Maybe some examples could be useful. This part of the manual is rather
terse.

Regards,

-- 
Nicolas Goaziou