Re: [O] [patch][ox-html] Support for level based containers

[Rasmus]

Hi,

Thanks for the—as always—useful comments.  I have attached a new
version of the patch with better doc and a bit better functionality, I
think.

Nicolas Goaziou <address@hidden> writes:

> I don't know enough HTML to have an opinion here.

Then we are two. . .

>> and what changes are necessary, if any.
>
> Some comments follow.
>
>> +(defcustom org-html-container-element '(("div" . "section")
>> +                                    ("div" . "article")
>> +                                    ("div" . "div"))
>> +  "HTML elements to use for wrapping sections.
>>  Can be set with the in-buffer HTML_CONTAINER property or for
>>  publishing, with :html-container.
>>
>> -Note that changing the default will prevent you from using
>> -org-info.js for your website."
>> +Should be a list of cons cells with positions corresponding to a
>
> A "list of cons cells" is an "alist".

I think I knew that at some point, but forgot along the way.  At least
"list of cons" is better than "list of tuples". . .

>> +section.  If `org-html-html5-fancy' is t the cdr is used
>> +otherwise the car.
>
> "is non-nil" is better than "is t". Also, you shouldn't use
> `org-html-html5-fancy': see below.

>> +Note that changing the default will prevent you from
>> +using org-info.js for your website."
>>    :group 'org-export-html
>>    :version "24.4"
>>    :package-version '(Org . "8.0")
>> -  :type 'string)
>> +  :type '(repeat (cons string string)))
>
> There is an `alist' type. See (info "(elisp) Composite Types")

I went for another solution as I would prefer not to break backward
comparability.

>> +      (let* ((hc (plist-get info :html-container))
>> +         (n (org-export-get-relative-level headline info)))
>
> You don't need a starred `let' here. Also, I suggest to not use short
> variable names. IMO "container-alist" is better than "hc" and "level"
> better than "n".

Good rule of thumb.

>> +    (cond ((listp hc)
>> +           (or (funcall (if org-html-html5-fancy 'cdr-safe 'car-safe)
>> +                        (nth (1- (min n (length hc))) hc)) "div"))
>
> You shouldn't use directly the variable `org-html-html5-fancy' since its
> value can be overridden with external properties (e.g., during
> a publishing process, with a specific setup). Instead, it should be:
>
>   (plist-get info :html-html5-fancy)
>
> As a rule of thumb, don't use variables when there's a property in INFO
> for them.

You are right.  Thanks for spotting it.

> Also, I don't think you need to use `car-safe' instead of `car' and
> `cdr-safe' instead of `cdr'.

My "defense" is, people could make mistakes, e.g. provide the list
'("div" "div" "div").  But you are right.

> Eventually, due to the (min n (length hc)) (which should be documented
> in the docstring) and the fact that the alist cannot be empty, the
> `funcall' never evaluates to nil. Therefore, the `or' is not necessary.

See below.

>> +          ((and (stringp hc) (= 1 n))
>> +           (plist-get info :html-container))
>
> Note that this branch is always false since HC shouldn't be a string,
> per the defcustom type, but an alist.

Yeah, but since ATM it's a string, there may be people who've
customized it in there init.el or in project definition.  I wouldn't
want to break it for them. . .  I have adjusted the docstring to
consider this.


>> +          (t "div")))))
>
> Given the recommendations above, the whole `cond' could be rewritten:
>
>   (funcall (if (plist-get info :html-html5-fancy) #'cdr #'car)
>            (nth (1- (min level (length container-alist))) container-alist))

More elegant. 

Thanks,
Rasmus

--
Hooray!

0001-Support-for-level-based-containers-in-ox-html.patch
Description: Text Data