Re: Byte-compilation of custom themes

[Eli Zaretskii]

> 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.

> 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.

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.

>      (dolist (dir (custom-theme--load-path))
> +      ;; `custom-theme--load-path' promises DIR exists.

"promises DIR exists and is a directory", I think.

>  (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."

Thanks.