Package: emacs;
Reported by: Troy Brown <brownts <at> troybrown.dev>
Date: Sun, 10 Mar 2024 20:33:02 UTC
Severity: normal
Found in version 30.0.50
Done: Eli Zaretskii <eliz <at> gnu.org>
Bug is archived. No further changes may be made.
View this message in rfc822 format
From: Troy Brown <brownts <at> troybrown.dev> To: Vladimir Kazanov <vekazanov <at> gmail.com> Cc: 69714 <at> debbugs.gnu.org Subject: bug#69714: 30.0.50; ert-font-lock doesn't handle list of faces Date: Wed, 13 Mar 2024 13:48:48 -0400
The new patch looks good to me. One other thing I thought I'd mention. I have places in my test where I want to verify that there is no font lock face. I've been able to specify "nil" for the face and that works perfectly to check this. I thought I'd mention this because I wasn't sure if it was intentional behavior, but I do find that useful. Thanks, Troy. On Wed, Mar 13, 2024 at 1:04 PM Vladimir Kazanov <vekazanov <at> gmail.com> wrote: > > Thanks a lot! > > The suggestions really do make sense. > > Here's the final integrated patch, complete with updated tests and > docs. If you're fine with it then I'll ask somebody to install it on > master. > > PS I've got to write an additional announcement in the main mailing > list inviting people to check the updated version out. > > On Wed, 13 Mar 2024 at 16:14, Troy Brown <brownts <at> troybrown.dev> wrote: > > > > Hi Vlad, sorry for the delayed response. > > > > I haven't pushed my change which uses this package yet, as I was > > struggling to get it working and didn't want to push failing tests. I > > just discovered the package and was working on a regression test for a > > bug fix involving font locking. This seemed like the perfect reason > > to use your package. At the moment I only have this one > > work-in-progress test, but I expect to use it more going forward. > > > > I did check out your patch and for my immediate needs, it worked > > perfectly. Thanks! Additionally, I did experiment a little with the > > multi-caret functionality, which is nice as I have a use for that. I > > also experimented with the negation functionality (although I don't > > have an immediate need for that), and did notice a couple things. The > > first was that the assertion would be ignored if there was a space > > between the negation symbol and the face. Also, if the actual and > > expected faces didn't match and the negation flag was being used, it > > acted like the negation was not indicated at all and failed the test. > > I've included a diff below containing the changes I made which seemed > > to address those concerns. > > > > diff --git a/lisp/emacs-lisp/ert-font-lock.el b/lisp/emacs-lisp/ert-font-lock.el > > index 06c90add9d3..1a5fe96fb09 100644 > > --- a/lisp/emacs-lisp/ert-font-lock.el > > +++ b/lisp/emacs-lisp/ert-font-lock.el > > @@ -61,6 +61,7 @@ ert-font-lock--assertion-line-re > > (group (zero-or-more (seq "^" (zero-or-more whitespace)))) > > ;; optional negation of the face specification > > (group (optional "!")) > > + (zero-or-more whitespace) > > ;; face symbol name or a list of symbols > > (group (or (regexp ert-font-lock--face-symbol-re) > > (regexp ert-font-lock--face-symbol-list-re)))) > > @@ -354,7 +355,7 @@ ert-font-lock--check-faces > > (when (symbolp expected-face) > > (setq expected-face (list expected-face))) > > > > - (when (not (equal actual-face expected-face)) > > + (when (and (not negation) (not (equal actual-face expected-face))) > > (ert-fail > > (list (format "Expected face %S, got %S on line %d column %d" > > expected-face actual-face line-checked column-checked) > > > > > > Thanks, > > > > Troy. > > > > On Tue, Mar 12, 2024 at 4:47 PM Vladimir Kazanov <vekazanov <at> gmail.com> wrote: > > > > > > Hi, > > > > > > I've attached a patch that handles face lists, fails on files without > > > assertions and expands the parser a bit to support multiple carets per > > > line. > > > > > > For faces it does the following: > > > > > > 1. Turn symbols into single element lists. > > > 2. Parses face lists from the assertions. > > > 3. Compare face lists using equas. > > > > > > Could you please check if this works for you? > > > > > > Thanks > > > > > > On Mon, 11 Mar 2024 at 08:36, Vladimir Kazanov <vekazanov <at> gmail.com> wrote: > > > > > > > > Hi, > > > > > > > > Thanks for reporting this! I have a bunch of ert-font-lock > > > > improvements in my local repo getting ready for submission, and can > > > > look into your suggestions as well. > > > > > > > > Do you have your unit test code somewhere in a public repo? It'd be > > > > great to think of further improvements to support your use case. > > > > > > > > Thanks, > > > > Vlad > > > > > > > > On Sun, 10 Mar 2024 at 20:33, Troy Brown <brownts <at> troybrown.dev> wrote: > > > > > > > > > > I'm trying to use this package to test out my tree-sitter mode, but am > > > > > running into an issue with lists of faces. It's possible that the > > > > > face for a location in the buffer will contain a list of 1 or more > > > > > faces. For example, when I use the ":override 'prepend" keyword in > > > > > the call to treesit-font-lock-rules, even if only a single face is > > > > > specified for the rule that matches that section of the buffer, this > > > > > will result in a list of one entry (i.e., "(face-name)"). > > > > > > > > > > When this happens, ert-font-lock fails to recognize that this matches > > > > > the face "face-name" (e.g., "^ face-name" will fail to match in this > > > > > case). I feel the tool should recognize a list containing a single > > > > > face as matching the face. Even worse however, it appears > > > > > ert-font-lock doesn't support a list of faces in the comment. I tried > > > > > to work around the original issue by using "^ (face-name)", but the > > > > > tool silently ignores this, as it doesn't match the internal regular > > > > > expression (which ended up allowing my test to pass without actually > > > > > checking anything). > > > > > > > > > > I can't figure out a way to use this tool in its current state due to > > > > > its lack of support for a list of faces. Also, I find that since it > > > > > silently ignores incorrect comment syntax (e.g., "^face-name", "^ > > > > > (face-name)"), it gives a false illusion that it's actually performing > > > > > those checks (and the checks are passing), when it's really just > > > > > ignoring them. Maybe any comment line starting with a "^" or "<-" > > > > > should be considered an assertion check and to fail if the rest of the > > > > > syntax is not as expected. Maybe it should also fail the test if no > > > > > assertion checks are found in a source file or string. > > > > > > > > > > Even if the tool would allow a list of a single face to match the > > > > > supplied face in the comment, I think it should also allow for > > > > > multiple faces to be listed in the comment. I have other places where > > > > > multiple faces are used (e.g., "(font-lock-constant-face > > > > > font-lock-variable-name-face)" to highlight a constant variable), > > > > > which would not be testable with the current state of the package. > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > Regards, > > > > > > > > Vladimir Kazanov > > > > > > > > > > > > -- > > > Regards, > > > > > > Vladimir Kazanov > > > > -- > Regards, > > Vladimir Kazanov
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.