[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Byte-compilation of custom themes
From: |
Eli Zaretskii |
Subject: |
Re: Byte-compilation of custom themes |
Date: |
Sat, 12 May 2018 10:04:10 +0300 |
> 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.