Subject: Re: bug#76372: 31.0.50; False () in ert-font-lock macro signatures

From: "Basil L. Contovounesios" <[email-masked]>
Date: Wed, 19 Feb 2025 21:15:50 +0100

Vladimir Kazanov [2025-02-18 19:41 +0000] wrote:

> Thanks for taking a look at ert-font-lock! I am on vacation right now
> so only have time for a brief review of the patch.

No worries.
I hope you have a nice vacation, and thanks for taking a look anyway.

> -     (insert ,str)
>       (,mode)
> +     (insert ,str)
>
> Not saying this is wrong but looks like something that might change
> things subtly. Do tests pass?

Yes, tests still pass (modulo those that were already failing):

$ make check
$ make TEST_LOAD_EL=no \
       test/{autoconf,{go,lua,rust}-ts-mode,ert{,-x,-font-lock}}-tests

If this were a general-purpose macro required to be robust, I would have
used e.g. macroexp-let2 to ensure the correct argument evaluation order.

Since this is a convenience used only in tests, I thought I could get
away with being lazy, but you caught me ;).

>> -  (skip-unless (featurep 'php-mode))
>> +  (skip-unless (fboundp 'php-mode))
>
> What difference does it make? featurep vs fboundp?

featurep returns non-nil if the file php-mode.el has already been
loaded, whereas fboundp returns non-nil if the function php-mode is
defined (even as an autoload).

During 'make check' it doesn't make a difference, since php-mode is not
even installed, let alone defined as a function or feature.

But if someone has php-mode.el installed via package-install, then the
function php-mode is automatically autoloaded.  If the test tries to
call the function php-mode, Emacs will know which file to load in order
to find the function's real definition.  So it is not necessary to have
php-mode.el already loaded (which involves an extra step by the user);
it suffices to have the php-mode package installed properly in order to
run the test.

Regardless of these details, fboundp is a more accurate test-skip
condition because all the test is doing is calling a function called
php-mode; it does not care about where that function is defined, and
specifically whether it's defined by a feature called php-mode.

>> -    "Test reading correct assertions from a file"
>> +  "Test reading correct assertions from a file"
>
> Similar to other docstrings, this asks for a period in the end.

Well spotted, thanks.

I've updated the patch locally, and am ready to apply further feedback,
or push it if there are no further comments.

-- 
Basil