I realise this is low priority stuff, but I'm nonetheless sorry for taking so long to respond. Eli Zaretskii writes: >> From: "Basil L. Contovounesios" >> Cc: , >> 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