Re: [PATCH v3] * tp/ext/highlight_syntax.pm: show warning if HIGHLIGHT_SYNTAX is defaulting to source-highlight since this behaviour will change in the near future. * tp/tests/other/list-of-tests: set HIGHLIGHT_SYNTAX=source-highlight

[Patrice Dumas]

On Wed, Dec 11, 2024 at 07:19:33PM +0000, Gavin Smith wrote:
> On Wed, Nov 27, 2024 at 05:31:15AM +0000, Carlos Maniero wrote:
> > ---
> > V3:
> > - Add missing entry on Changelog
> > - Update the commit message with the Changelog entry
> > 
> > Thanks Patrice for the feedback!
> > 
> >  ChangeLog                    | 7 +++++++
> >  tp/ext/highlight_syntax.pm   | 5 +++++
> >  tp/tests/other/list-of-tests | 4 ++--
> >  3 files changed, 14 insertions(+), 2 deletions(-)
>  
> 
> It was suggested that this change be made before the next release.
> 
> However, I do not like the idea of saying that HIGHLIGHT_SYNTAX has an
> invalid value, and yet setting the value still does something (causes
> "source-highlight") to be used.  It seems more straightforward to make
> it so that an invalid value is as if the variable were not set at all.
> 
> Here is a proposed patch to make this change.  (The changes to the
> tests made sense.)  I move most of the texinfo_register_* calls
> into 'highlight_setup' so that the module will not be active if
> HIGHLIGHT_SYNTAX has an unrecognised value.

If highlight_setup returns early, %highlighted_languages_list remains
empty, $commands{$cmdname}->{'results'} is never set, and therefore the
registered commands formatting functions call the default functions.
Therefore I see little gain in having texinfo_register_* calls only if
HIGHLIGHT_SYNTAX has a recognized value, to me is should be treated as
any other error that leaves %highlighted_languages_list empty.

This would also allow to have texinfo_register_* be called independently
of highlight_setup having succeeded.  There is no requirement for that,
but it is the general style of using init files, the functions are
always registered, but call the defaults if the initialization did not
proceed correctly.

The remainder of the patch looks ok to me.

> 
> I have not committed this as I am not very familiar with the texi2any
> Perl API so the patch would benefit from someone checking if it was
> correct.
> 
> diff --git a/tp/ext/highlight_syntax.pm b/tp/ext/highlight_syntax.pm
> index 953f5c10eb..c760d652e3 100644
> --- a/tp/ext/highlight_syntax.pm
> +++ b/tp/ext/highlight_syntax.pm
> @@ -54,14 +54,6 @@ my %languages_extensions = (
>  my %highlighted_languages_list;
>  
>  texinfo_register_handler('setup', \&highlight_setup);
> -texinfo_register_handler('structure', \&highlight_process);
> -texinfo_register_command_formatting('example', 
> \&highlight_preformatted_command);
> -# normally this is done in preformatted type, but preformatted
> -# types conversion output in example is discarded in
> -# highlight_preformatted_command, so register a replacement.
> -# Register inline pending content when opening an example block.
> -texinfo_register_command_opening('example',
> -                                 \&highlight_open_inline_container_type);
>  
>  sub highlight_setup($$)
>  {
> @@ -74,15 +66,22 @@ sub highlight_setup($$)
>  
>    my $highlight_type = $self->get_conf('HIGHLIGHT_SYNTAX');
>  
> +  return 1 if !defined($highlight_type);
> +
>    my $cmd;
> -  if (defined($highlight_type) and $highlight_type eq 'highlight') {
> +  if ($highlight_type eq 'highlight') {
>      $cmd = 'highlight --list-scripts=lang';
> -  } elsif (defined($highlight_type) and $highlight_type eq 'pygments') {
> +  } elsif ($highlight_type eq 'pygments') {
>      $cmd = 'pygmentize -L lexers';
> -  } else {
> +  } elsif ($highlight_type eq 'source-highlight') {
>      $highlight_type = 'source-highlight';
>      $cmd = 'source-highlight --lang-list';
> +  } else {
> +    $self->converter_document_warn(sprintf(__(
> +      "`%s' is not valid for HIGHLIGHT_SYNTAX"), $highlight_type));
> +    return 1;
>    }
> +
>    if ($highlight_type_languages_name_mappings{$highlight_type}) {
>      %languages_name_mapping
>        = %{$highlight_type_languages_name_mappings{$highlight_type}};
> @@ -90,6 +89,15 @@ sub highlight_setup($$)
>      %languages_name_mapping = ();
>    }
>  
> +  texinfo_register_handler('structure', \&highlight_process);
> +  texinfo_register_command_formatting('example', 
> \&highlight_preformatted_command);
> +  # normally this is done in preformatted type, but preformatted
> +  # types conversion output in example is discarded in
> +  # highlight_preformatted_command, so register a replacement.
> +  # Register inline pending content when opening an example block.
> +  texinfo_register_command_opening('example',
> +                                   \&highlight_open_inline_container_type);
> +
>    # NOTE open failure triggers a warning message if run with -w if the
>    # file is not found.  This message can be catched with $SIG{__WARN__}.
>    # This message is along:
>