From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nicolas Goaziou Subject: Re: [ox-publish, patch] More flexible sitemaps Date: Wed, 01 Jun 2016 17:34:56 +0200 Message-ID: <87wpm8j527.fsf@saiph.selenimh> References: <87eg8ydpli.fsf@gmx.us> <87twhpk8e1.fsf@saiph.selenimh> <87h9djv4gw.fsf@gmx.us> Mime-Version: 1.0 Content-Type: text/plain Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:36633) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b88Ao-0008Lc-N2 for emacs-orgmode@gnu.org; Wed, 01 Jun 2016 11:35:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b88Ai-00024h-S5 for emacs-orgmode@gnu.org; Wed, 01 Jun 2016 11:35:09 -0400 Received: from relay4-d.mail.gandi.net ([217.70.183.196]:41123) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b88Ai-00023m-Fr for emacs-orgmode@gnu.org; Wed, 01 Jun 2016 11:35:04 -0400 In-Reply-To: <87h9djv4gw.fsf@gmx.us> (rasmus@gmx.us's message of "Fri, 27 May 2016 18:41:03 +0200") 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" To: Rasmus Cc: emacs-orgmode@gnu.org Hello, Rasmus 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