GNU bug report logs -
#76372
31.0.50; False () in ert-font-lock macro signatures
Previous Next
Reported by: "Basil L. Contovounesios" <basil <at> contovou.net>
Date: Mon, 17 Feb 2025 20:38:02 UTC
Severity: normal
Tags: patch
Found in versions 31.0.50, 30.0.93
Fixed in version 31.1
Done: "Basil L. Contovounesios" <basil <at> contovou.net>
Bug is archived. No further changes may be made.
Full log
View this message in rfc822 format
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
This bug report was last modified 129 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.