Re: Byte-compilation of custom themes

[Basil L. Contovounesios]

I realise this is low priority stuff, but I'm nonetheless sorry for
taking so long to respond.

Eli Zaretskii <address@hidden> writes:

>> From: "Basil L. Contovounesios" <address@hidden>
>> Cc: <address@hidden>,  <address@hidden>
>> Date: Fri, 11 May 2018 21:43:42 +0100
>> 
>> > We should at least have a comment there that we are relying on
>> > custom-theme--load-path to do the test, and perhaps also an assertion.
>> 
>> Do you mean a cl-assertion, or an emulation thereof?
>
> I meant cl-assert.

Unless I (being unfamiliar with the subtleties of the build and
bootstrap process) am missing something, I think it's too early to load
cl-lib here; at least 'make bootstrap' fails for me when I add
(eval-when-compile (require 'cl-lib)) to custom.el.  Is there a way
around this?  Is the (unless DIRP (signal 'file-missing ...)) spiel from
my last email a suitable alternative?  Would doing that be overkill?

>> Either way, what is the benefit of barfing before directory-files does?
>
> That you can control the text of the error message, and make it more
> user-friendly.  Also, an assertion clearly means we intended this not
> to happen, rather than that the code failed to handle some valid
> situation.

Understood.  I've noticed, however, that many, if not most, uses of
cl-assert don't specify a custom error message.  For future reference,
is this acceptable practice, or would we rather see consistent use of
more informative messages?

> Once again, the minimum I requested was a comment; it's up to you
> whether to use cl-assert.  But see below.
>
>> Wouldn't a docstring for custom-theme--load-path and a comment in
>> custom-available-themes suffice for the reader (they do for me)?
>
> We've seen many situations where code guidelines are violated, for
> whatever reasons, so just documenting stuff is not enough.  Moreover,
> custom-theme--load-path might some day change and invalidate our
> assumption.  The way to prevent that from happening could be either
> having the assertion in the caller, or by adding a test to the test
> suite that makes sure the list returned by custom-theme--load-path
> never includes anything but existing directories.

A limitation (which does not apply in this case) of having the assertion
in the caller is that this doesn't necessarily cover future callers, and
even if it does, then there are multiple callers checking the same
thing.  Either way, I think adding a regression test is a good idea.

>>      (dolist (dir (custom-theme--load-path))
>> +      ;; `custom-theme--load-path' promises DIR exists.
>
> "promises DIR exists and is a directory", I think.

Right, I was being lazy and imprecise by relying on the name DIR to
convey its being a directory.

>>  (defun custom-theme--load-path ()
>> +  "Expand `custom-theme-load-path' into list of directories.
>> +Only existing directories are included in the path returned."
>
> I'd find this text a bit of a riddle.  How about this instead:
>
>     "Expand `custom-theme-load-path' into list of directories.
>   Members of `custom-theme-load-path' that either don't exist or are not
>   directories are omitted from the expansion."

SGTM, thanks.

I reattach the eight patches, updated for the feedback above.

The third patch has been updated to include the aforementioned
custom-theme-load-path documentation and tests.

The eighth patch now touches two more subr-x.el functions and adds tests
for its changes.

Thanks,

-- 
Basil

0001-Improve-loading-of-byte-compiled-custom-themes.patch
Description: Text Data

0002-Disable-no-byte-compile-in-built-in-themes.patch
Description: Text Data

0003-Fix-custom-available-themes-file-expansion.patch
Description: Text Data

0004-lisp-custom.el-Use-lexical-binding.patch
Description: Text Data

0005-lisp-cus-theme.el-Use-lexical-binding.patch
Description: Text Data

0006-Minor-custom.el-simplifications.patch
Description: Text Data

0007-Minor-cus-theme.el-simplifications.patch
Description: Text Data

0008-Tweak-subr-x.el-substring-functions.patch
Description: Text Data