GNU bug report logs - #64275
30.0.50; [PATCH] Improve sigil font-lock match for elixir-ts-mode

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 64275 in the body.
You can then email your comments to 64275 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 30.0.50; [PATCH] Improve sigil font-lock match for elixir-ts-mode
Date: Sat, 24 Jun 2023 22:14:17 +0200

[Message part 1 (text/plain, inline)]

This patch updates the sigil matching for Elixir.  We don't need 
to know
what the sigil is to be able to set the font.  The one exception 
is
regex via a sigil.

[0001-Fix-eglot-imenu-when-the-server-does-not-support-it.patch (text/x-patch, attachment)]

[Message part 3 (text/plain, inline)]

Wilhelm

Message #8 received at 64275 <at> debbugs.gnu.org (full text, mbox):

From: Basil Contovounesios <contovob <at> tcd.ie>
To: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
Cc: 64275 <at> debbugs.gnu.org
Subject: Re: bug#64275: 30.0.50; [PATCH] Improve sigil font-lock match for
 elixir-ts-mode
Date: Sun, 25 Jun 2023 09:53:55 +0100

Wilhelm Kirschbaum [2023-06-24 22:14 +0200] wrote:

> This patch updates the sigil matching for Elixir.  We don't need to know
> what the sigil is to be able to set the font.  The one exception is
> regex via a sigil.
>
> From 9a1389305d92f0e08d39d2ff5540cb494c012f12 Mon Sep 17 00:00:00 2001
> From: Wilhelm H Kirschbaum <wkirschbaum <at> gmail.com>
> Date: Sat, 24 Jun 2023 21:54:30 +0200
> Subject: [PATCH 1/1] Fix eglot-imenu when the server does not support it

Looks like the patch from bug#64274 was attached instead.

-- 
Basil

Message #11 received at 64275 <at> debbugs.gnu.org (full text, mbox):

From: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
To: Basil Contovounesios <contovob <at> tcd.ie>
Cc: 64275 <at> debbugs.gnu.org
Subject: Re: bug#64275: 30.0.50; [PATCH] Improve sigil font-lock match for
 elixir-ts-mode
Date: Sun, 25 Jun 2023 11:00:44 +0200

[Message part 1 (text/plain, inline)]

> Looks like the patch from bug#64274 was attached instead.

Ah, sorry. Here is the correct patch.

Wilhelm

[0001-Update-sigil-font-lock-match-for-elixir-ts-mode.patch (text/x-patch, attachment)]

Message #14 received at 64275 <at> debbugs.gnu.org (full text, mbox):

From: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
To: Basil Contovounesios <contovob <at> tcd.ie>
Cc: 64275 <at> debbugs.gnu.org
Subject: Re: bug#64275: 30.0.50; [PATCH] Improve sigil font-lock match for
 elixir-ts-mode
Date: Sat, 29 Jul 2023 20:31:05 +0200

Wilhelm Kirschbaum <wkirschbaum <at> gmail.com> writes:

>> Looks like the patch from bug#64274 was attached instead.
>
> Ah, sorry. Here is the correct patch.
>
> Wilhelm
>
> [2. text/x-patch; 
> 0001-Update-sigil-font-lock-match-for-elixir-ts-mode.patch]...

Can this patch still be installed please?

Message #17 received at 64275 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>,
 Basil Contovounesios <contovob <at> tcd.ie>
Cc: 64275 <at> debbugs.gnu.org
Subject: Re: bug#64275: 30.0.50; [PATCH] Improve sigil font-lock match for
 elixir-ts-mode
Date: Sun, 30 Jul 2023 05:06:37 +0300

On 25/06/2023 12:00, Wilhelm Kirschbaum wrote:
>         "~" @font-lock-string-face
>         (sigil_name) @elixir-ts-font-sigil-name-face
>         (:match "^[rR]$" @elixir-ts-font-sigil-name-face))
>        @font-lock-regexp-face
> -     (sigil
> -      "~" @font-lock-string-face
> -      (sigil_name) @elixir-ts-font-sigil-name-face
> -      quoted_start: _ @font-lock-string-face
> -      quoted_end: _ @font-lock-string-face
> -      (:match "^[HF]$" @elixir-ts-font-sigil-name-face)))
> -
> +     (sigil) @font-lock-string-face)

Hi Wilhelm!

Should we continue to use elixir-ts-font-sigil-name-face, though?

With the new patch, it will continue highlight the sigil name ("r") in 
regexps, but no in other sigil types (s/c/w).

What's your opinion on that?

Message #20 received at 64275 <at> debbugs.gnu.org (full text, mbox):

From: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
To: Dmitry Gutov <dmitry <at> gutov.dev>
Cc: Basil Contovounesios <contovob <at> tcd.ie>, 64275 <at> debbugs.gnu.org
Subject: Re: bug#64275: 30.0.50; [PATCH] Improve sigil font-lock match for
 elixir-ts-mode
Date: Sun, 30 Jul 2023 09:53:15 +0200

Dmitry Gutov <dmitry <at> gutov.dev> writes:

> On 25/06/2023 12:00, Wilhelm Kirschbaum wrote:
>>         "~" @font-lock-string-face
>>         (sigil_name) @elixir-ts-font-sigil-name-face
>>         (:match "^[rR]$" @elixir-ts-font-sigil-name-face))
>>        @font-lock-regexp-face
>> -     (sigil
>> -      "~" @font-lock-string-face
>> -      (sigil_name) @elixir-ts-font-sigil-name-face
>> -      quoted_start: _ @font-lock-string-face
>> -      quoted_end: _ @font-lock-string-face
>> -      (:match "^[HF]$" @elixir-ts-font-sigil-name-face)))
>> -
>> +     (sigil) @font-lock-string-face)
>
> Hi Wilhelm!
>
> Should we continue to use elixir-ts-font-sigil-name-face, 
> though?
>

Hi :)

We still need to match only the sigil_name part, so can't use the
@font-lock-string-face as it will exclude the '~', therefore we 
need a
custom font for the match.  This is only true for ~r/regex content 
here/ and
~R/regex content here/, or variance ~r(regex content here) 
etc. where
the font for the regex part is different than the font for the 
sigil
prefix.

Perhaps there is another way to both apply a font with
@font-lock-string-face and exclude it from the :match, but last 
time I
checked this was not possible.

> With the new patch, it will continue highlight the sigil name 
> ("r") in
> regexps, but no in other sigil types (s/c/w).
>

Incorrect, just below there is a:

          (sigil) @font-lock-string-face)

without a match, so they will be highlighted, but just more 
generically
as we don't know which sigils the user might have assigned.

> What's your opinion on that?

As a baseline it makes sense to me to apply font-lock-string-face 
to all
sigils, as they can be user defined.  Some are language core 
sigils,
like the ~r and ~R sigils and then there are common library 
accepted
sigils like ~H and ~F which can effectively be seen as part of the
language, these can be added over time as we improve this mode. 
The
initial ideas was to try and match a complete list, but won't 
work.
Users can also define sigils in their projects, so its better to 
rely on
the grammar to tell us its a sigil.

I ran the current patch for over a month and feels correct.

Wilhelm

Message #23 received at 64275 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
Cc: Basil Contovounesios <contovob <at> tcd.ie>, 64275 <at> debbugs.gnu.org
Subject: Re: bug#64275: 30.0.50; [PATCH] Improve sigil font-lock match for
 elixir-ts-mode
Date: Sun, 30 Jul 2023 15:09:51 +0300

On 30/07/2023 10:53, Wilhelm Kirschbaum wrote:
>> With the new patch, it will continue highlight the sigil name ("r") in
>> regexps, but no in other sigil types (s/c/w).
>>
> 
> Incorrect, just below there is a:
> 
>            (sigil) @font-lock-string-face)
> 
> without a match, so they will be highlighted, but just more generically
> as we don't know which sigils the user might have assigned.

Why not add elixir-ts-font-sigil-name-face in that matcher as well?

Message #26 received at 64275 <at> debbugs.gnu.org (full text, mbox):

From: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
To: Dmitry Gutov <dmitry <at> gutov.dev>
Cc: Basil Contovounesios <contovob <at> tcd.ie>, 64275 <at> debbugs.gnu.org
Subject: Re: bug#64275: 30.0.50; [PATCH] Improve sigil font-lock match for
 elixir-ts-mode
Date: Mon, 31 Jul 2023 09:22:40 +0200

Dmitry Gutov <dmitry <at> gutov.dev> writes:

> On 30/07/2023 10:53, Wilhelm Kirschbaum wrote:
>>> With the new patch, it will continue highlight the sigil name 
>>> ("r") in
>>> regexps, but no in other sigil types (s/c/w).
>>>
>> Incorrect, just below there is a:
>>            (sigil) @font-lock-string-face)
>> without a match, so they will be highlighted, but just more
>> generically
>> as we don't know which sigils the user might have assigned.
>
> Why not add elixir-ts-font-sigil-name-face in that matcher as 
> well?

I think it is worth keeping the default/fallback case as simple as
possible and if there is a need to add special font to more 
specific
cases.  In this case it would have no impact if the font is not
customized.  Is there a reason you believe it would be better?

Message #29 received at 64275 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
Cc: Basil Contovounesios <contovob <at> tcd.ie>, 64275 <at> debbugs.gnu.org
Subject: Re: bug#64275: 30.0.50; [PATCH] Improve sigil font-lock match for
 elixir-ts-mode
Date: Tue, 1 Aug 2023 14:59:18 +0300

On 31/07/2023 10:22, Wilhelm Kirschbaum wrote:
> I think it is worth keeping the default/fallback case as simple as
> possible and if there is a need to add special font to more specific
> cases.

But the font is called elixir-ts-font-sigil-name-face. It should apply 
to all sigils, shouldn't it?

At least if we just go by the name.

Message #32 received at 64275 <at> debbugs.gnu.org (full text, mbox):

From: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
To: Dmitry Gutov <dmitry <at> gutov.dev>
Cc: Basil Contovounesios <contovob <at> tcd.ie>, 64275 <at> debbugs.gnu.org
Subject: Re: bug#64275: 30.0.50; [PATCH] Improve sigil font-lock match for
 elixir-ts-mode
Date: Wed, 02 Aug 2023 08:38:39 +0200

Dmitry Gutov <dmitry <at> gutov.dev> writes:

> On 31/07/2023 10:22, Wilhelm Kirschbaum wrote:
>> I think it is worth keeping the default/fallback case as simple 
>> as
>> possible and if there is a need to add special font to more 
>> specific
>> cases.
>
> But the font is called elixir-ts-font-sigil-name-face. It should 
> apply
> to all sigils, shouldn't it?
>
> At least if we just go by the name.

Sure, but need to tweak it a bit when I have time.  Maybe there is 
a way
to match in the query without applying a font, then it will look a 
bit
more sensible.

Message #35 received at 64275 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
Cc: Basil Contovounesios <contovob <at> tcd.ie>, 64275 <at> debbugs.gnu.org
Subject: Re: bug#64275: 30.0.50; [PATCH] Improve sigil font-lock match for
 elixir-ts-mode
Date: Wed, 16 Aug 2023 05:11:44 +0300

On 02/08/2023 09:38, Wilhelm Kirschbaum wrote:
> 
> Dmitry Gutov <dmitry <at> gutov.dev> writes:
> 
>> On 31/07/2023 10:22, Wilhelm Kirschbaum wrote:
>>> I think it is worth keeping the default/fallback case as simple as
>>> possible and if there is a need to add special font to more specific
>>> cases.
>>
>> But the font is called elixir-ts-font-sigil-name-face. It should apply
>> to all sigils, shouldn't it?
>>
>> At least if we just go by the name.
> 
> Sure, but need to tweak it a bit when I have time.  Maybe there is a way
> to match in the query without applying a font, then it will look a bit
> more sensible.

I'm not sure what you meant by the last sentence, sorry. What do you 
think about the below?

  (sigil (sigil_name) @elixir-ts-font-sigil-name-face) 
@font-lock-string-face)

Message #38 received at 64275 <at> debbugs.gnu.org (full text, mbox):

From: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
To: Dmitry Gutov <dmitry <at> gutov.dev>
Cc: Basil Contovounesios <contovob <at> tcd.ie>, 64275 <at> debbugs.gnu.org
Subject: Re: bug#64275: 30.0.50; [PATCH] Improve sigil font-lock match for
 elixir-ts-mode
Date: Sat, 07 Oct 2023 10:46:57 +0200

[Message part 1 (text/plain, inline)]

Dmitry Gutov <dmitry <at> gutov.dev> writes:

> On 02/08/2023 09:38, Wilhelm Kirschbaum wrote:
>> Dmitry Gutov <dmitry <at> gutov.dev> writes:
>> 
>>> On 31/07/2023 10:22, Wilhelm Kirschbaum wrote:
>>>> I think it is worth keeping the default/fallback case as 
>>>> simple as
>>>> possible and if there is a need to add special font to more 
>>>> specific
>>>> cases.
>>>
>>> But the font is called elixir-ts-font-sigil-name-face. It 
>>> should apply
>>> to all sigils, shouldn't it?
>>>
>>> At least if we just go by the name.
>> Sure, but need to tweak it a bit when I have time.  Maybe there 
>> is a
>> way
>> to match in the query without applying a font, then it will 
>> look a bit
>> more sensible.
>
> I'm not sure what you meant by the last sentence, sorry. What do 
> you
> think about the below?
>
>   (sigil (sigil_name) @elixir-ts-font-sigil-name-face)
>   @font-lock-string-face)

Sorry for the late reply.  I believe the following patch is more
appropriate as it will apply the sigil font to the entire sigil, 
but
also respect the regex face.  We can add string specific matches 
later,
but happy just to simplify what we have and fix apply appropriate 
font
to previously unmatched sigils.

[0001-Simplify-sigil-font-lock-match-for-elixir-ts-mode.patch (text/x-patch, attachment)]

Message #41 received at 64275 <at> debbugs.gnu.org (full text, mbox):

From: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
To: Dmitry Gutov <dmitry <at> gutov.dev>
Cc: Basil Contovounesios <contovob <at> tcd.ie>, 64275 <at> debbugs.gnu.org
Subject: Re: bug#64275: 30.0.50; [PATCH] Improve sigil font-lock match for
 elixir-ts-mode
Date: Sat, 07 Oct 2023 11:59:36 +0200

[Message part 1 (text/plain, inline)]

>>
>> I'm not sure what you meant by the last sentence, sorry. What 
>> do you
>> think about the below?
>>
>>   (sigil (sigil_name) @elixir-ts-font-sigil-name-face)
>>   @font-lock-string-face)
>
> Sorry for the late reply.  I believe the following patch is more
> appropriate as it will apply the sigil font to the entire sigil, 
> but
> also respect the regex face.  We can add string specific matches
> later,
> but happy just to simplify what we have and fix apply 
> appropriate font
> to previously unmatched sigils.
>
> [2. text/x-patch; 
> 0001-Simplify-sigil-font-lock-match-for-elixir-ts-mode.patch]...

Actually, the above patch breaks the embedded HEEx sigils' font. 
We
have to exclude it when doing a general match.  Attached works 
with ~H
sigils as well.  Please ignore the previous patch and install this 
one.

[0001-Simplify-sigil-font-lock-match-for-elixir-ts-mode(1).patch (text/x-patch, attachment)]

Message #44 received at 64275 <at> debbugs.gnu.org (full text, mbox):

From: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
To: Dmitry Gutov <dmitry <at> gutov.dev>
Cc: Basil Contovounesios <contovob <at> tcd.ie>, 64275 <at> debbugs.gnu.org
Subject: Re: bug#64275: 30.0.50; [PATCH] Improve sigil font-lock match for
 elixir-ts-mode
Date: Sat, 07 Oct 2023 12:10:43 +0200

Wilhelm Kirschbaum <wkirschbaum <at> gmail.com> writes:

>>>
>>> I'm not sure what you meant by the last sentence, sorry. What 
>>> do
>>> you
>>> think about the below?
>>>
>>>   (sigil (sigil_name) @elixir-ts-font-sigil-name-face)
>>>   @font-lock-string-face)
>>
>> Sorry for the late reply.  I believe the following patch is 
>> more
>> appropriate as it will apply the sigil font to the entire 
>> sigil, but
>> also respect the regex face.  We can add string specific 
>> matches
>> later,
>> but happy just to simplify what we have and fix apply 
>> appropriate
>> font
>> to previously unmatched sigils.
>>
>> [2. text/x-patch;
>> 0001-Simplify-sigil-font-lock-match-for-elixir-ts-mode.patch]...
>
> Actually, the above patch breaks the embedded HEEx sigils' 
> font. We
> have to exclude it when doing a general match.  Attached works 
> with ~H
> sigils as well.  Please ignore the previous patch and install 
> this
> one.
>
> [2. text/x-patch; 
> 0001-Simplify-sigil-font-lock-match-for-elixir-ts-mode(1).patch]...


Sorry :(. I am finding more edge cases.  Maybe let me test it this
properly this week before we make any changes.

Message #47 received at 64275 <at> debbugs.gnu.org (full text, mbox):

From: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
To: Dmitry Gutov <dmitry <at> gutov.dev>
Cc: Basil Contovounesios <contovob <at> tcd.ie>, 64275 <at> debbugs.gnu.org
Subject: Re: bug#64275: 30.0.50;
 [PATCH] Improve sigil font-lock match for elixir-ts-mode
Date: Sat, 21 Oct 2023 09:39:38 +0200

[Message part 1 (text/plain, inline)]

> >>>
> >>> I'm not sure what you meant by the last sentence, sorry. What
> >>> do
> >>> you
> >>> think about the below?
> >>>
> >>>   (sigil (sigil_name) @elixir-ts-font-sigil-name-face)
> >>>   @font-lock-string-face)
> >>
> >> Sorry for the late reply.  I believe the following patch is
> >> more
> >> appropriate as it will apply the sigil font to the entire
> >> sigil, but
> >> also respect the regex face.  We can add string specific
> >> matches
> >> later,
> >> but happy just to simplify what we have and fix apply
> >> appropriate
> >> font
> >> to previously unmatched sigils.
> >>
> >> [2. text/x-patch;
> >> 0001-Simplify-sigil-font-lock-match-for-elixir-ts-mode.patch]...
> >
> > Actually, the above patch breaks the embedded HEEx sigils'
> > font. We
> > have to exclude it when doing a general match.  Attached works
> > with ~H
> > sigils as well.  Please ignore the previous patch and install
> > this
> > one.
> >
> > [2. text/x-patch;
> > 0001-Simplify-sigil-font-lock-match-for-elixir-ts-mode(1).patch]...
>
>
> Sorry :(. I am finding more edge cases.  Maybe let me test it this
> properly this week before we make any changes.
>

I tested this patch for a while now and it looks like the best approach
for handling regex sigils, but with an appropriate fallback.

If this can be installed, it will be appreciated.

[Message part 2 (text/html, inline)]

[0001-Simplify-sigil-font-lock-match-for-elixir-ts-mode.patch (text/x-patch, attachment)]

Message #52 received at 64275-done <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
Cc: Basil Contovounesios <contovob <at> tcd.ie>, 64275-done <at> debbugs.gnu.org
Subject: Re: bug#64275: 30.0.50; [PATCH] Improve sigil font-lock match for
 elixir-ts-mode
Date: Thu, 26 Oct 2023 03:27:13 +0300

On 21/10/2023 10:39, Wilhelm Kirschbaum wrote:
> 
>      >>>
>      >>> I'm not sure what you meant by the last sentence, sorry. What
>      >>> do
>      >>> you
>      >>> think about the below?
>      >>>
>      >>>   (sigil (sigil_name) @elixir-ts-font-sigil-name-face)
>      >>>   @font-lock-string-face)
>      >>
>      >> Sorry for the late reply.  I believe the following patch is
>      >> more
>      >> appropriate as it will apply the sigil font to the entire
>      >> sigil, but
>      >> also respect the regex face.  We can add string specific
>      >> matches
>      >> later,
>      >> but happy just to simplify what we have and fix apply
>      >> appropriate
>      >> font
>      >> to previously unmatched sigils.
>      >>
>      >> [2. text/x-patch;
>      >> 0001-Simplify-sigil-font-lock-match-for-elixir-ts-mode.patch]...
>      >
>      > Actually, the above patch breaks the embedded HEEx sigils'
>      > font. We
>      > have to exclude it when doing a general match.  Attached works
>      > with ~H
>      > sigils as well.  Please ignore the previous patch and install
>      > this
>      > one.
>      >
>      > [2. text/x-patch;
>      > 0001-Simplify-sigil-font-lock-match-for-elixir-ts-mode(1).patch]...
> 
> 
>     Sorry :(. I am finding more edge cases.  Maybe let me test it this
>     properly this week before we make any changes.
> 
> 
> I tested this patch for a while now and it looks like the best approach
> for handling regex sigils, but with an appropriate fallback.
> 
> If this can be installed, it will be appreciated.

Thanks! Installed, and closing.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.