GNU bug report logs - #78994
31.0.50; [PATCH] php-ts-mode: `php-ts-mode' depends on `mhtml-ts-mode' instead of JS,CSS and HTML.

Previous Next

Full log

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Vincenzo Pupillo <v.pupillo <at> gmail.com>
Cc: 78994 <at> debbugs.gnu.org
Subject: Re: bug#78994: 31.0.50;
 [PATCH] php-ts-mode: `php-ts-mode' depends on `mhtml-ts-mode' instead
 of JS,CSS and HTML.
Date: Tue, 05 Aug 2025 16:47:55 +0300

> Date: Tue, 05 Aug 2025 12:59:45 +0200
> From: Vincenzo Pupillo <v.pupillo <at> gmail.com>
> 
> Eli, any comments on this patch?

I hoped some user of this mode will be able to review, but I guess
that's not going to happen...

The patch is massive, and the changes quite radical.  But OTOH you are
the author and the maintainer of the mode, so who am I to question
your decisions, especially as I don't program in PHP?

So just a few superficial comments below, mainly about documentation:

> The direct dependence on js-ts-mode, css-ts-mode and html-ts-mode has
> now been replaced by ‘mhtml-ts-mode’.
> Additional benefits are:
> 1. Imenu now exposes symbols for all of all languages,
> 2. navigation now works correctly for all languages,
> 3. outline works for all languages.
> 
> Additional new features are:
> 1. indentation of PHP in mixed buffers with HTML now works much better
> and allows three different behaviors, an option allows you to choose the
> behavior.
> 2. a new feature shows where PHP ini files are both locally and
> remotely, if the buffer is associated with a remote PHP file.

I think at least part of the above should be in NEWS, to explain the
rationale for these changes.

> +*** The option 'php-ts-mode-css-fontify-colors' has been removed.
> +'mhtml-ts-mode-css-fontify-colors' replace this option.
                                      ^^^^^^^
"replaces"

> +*** New user option 'php-ts-mode-find-sibling-rules'.
> +Rules for finding siblings of a PHP files.
                                 ^^^^^^^^^^^
"a PHP file", singular, I believe.

> +*** New user option 'php-ts-mode-phpdoc-highlight-errors'.
> +When not null, it highlights unknown PHPDOC tags using
        ^^^^^^^^
Did you mean "non-nil"?

> +‘font-lock-warning-face’ so that the user can identify them more easily.
   ^^^^^^^^^^^^^^^^^^^^^^^^
Please quote in NEWS 'like this', using ASCII characters.

> +*** New command 'php-ts-mode-show-ini' to show where are located the ini files.

I would delete the part beginning with "to show", since you explain
that in the very next sentence:

> +Show the location of the PHP ini files, if the buffer is associated to a remote
> +PHP file show the remote PHP ini files.


> +(defcustom php-ts-mode-html-indent-offset 2
> +  "PHP indent spaces related to the HTML tags.

I think you mean

  The number of spaces to indent PHP code relative to HTML tags.

> +By default should have same value as `mhtml-ts-mode-js-css-indent-offset'."

Only "by default"?  I'm not sure I understand what you mean by "by
default should" -- do you mean it is not recommended to make it
different from mhtml-ts-mode-js-css-indent-offset?

> +(defcustom php-ts-mode-html-relative-indent t
> +  "How PHP code are indented relative to the HTML tags.
              ^^^^^^^^
Either "codes are" or "code is".

> +  :type '(choice (const nil) (const t) (const ignore))

Please consider adding :tag's to the 3 supported values, because the
values themselves don't really explain their effect.

> +(defcustom php-ts-mode-php-default-executable (or (executable-find "php") "php")
>    "The default PHP executable."

Suggest to clarify:

  "The default file name of the PHP executable."

> +(defcustom php-ts-mode-find-sibling-rules
> +  (list (list (rx "src/" (group (+ not-newline) "/") (group (+ (not "/"))) ".php") "tests/\\1\\2Test.php")
> +	(list (rx "tests/" (group (+ not-newline) "/") (group (+ (not "/"))) "Test.php") "src/\\1\\2.php"))
> +  "Rules for finding sibling files as defined by `find-sibling-rules'.
> +As a default, the rules try to find the corresponding test of the
> +current source file and vice versa.  Source files are assumed to be in
> +src/, and tests of in tests/.  Many frameworks have a folder
> +arrangement similar to this."

Please document in the doc string the form of the value.  Something
like

  Value should be a list whose elements are of the form

      (FILENAME-REGEXP SIBLING-FILE)

  where FILENAME-REGEXP is the regexp matching...

> +(defcustom php-ts-mode-phpdoc-highlight-errors nil
> +  "Highlights tags unknown to the phpdoc parser.
      ^^^^^^^^^^
"Highlight", without the "s".

> +(defun php-ts-mode-show-ini ()
> +  "Show PHP configuration file names."

Suggest to rephrase:

  Pop a buffer showing the PHP configuration file names.

> +Returns 0 if the NODE is after the </html>.  If
   ^^^^^^^
"Return"

> +`php-ts-mode-html-relative-indent' is not nil returns the indentation
                                                 ^^^^^^^
"return"

> +point of the last word before the NODE, plus the indentation offset,
> +otherwise return only the indentation point.  If there is no HTML tag,
> +it returns the beginning of the parent.  When NODE is nil it behave
                                                                ^^^^^^
"behaves"

Previous Next

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