Subject: Re: bug#71380: 30.0.50; Submitting php-ts-mode, new major mode for php

From: Eli Zaretskii <[email-masked]>
Date: Thu, 06 Jun 2024 09:58:11 +0300

> From: Vincenzo Pupillo <[email-masked]>
> Date: Wed, 05 Jun 2024 15:59:20 +0200
> 
> I would like to submit php-ts-mode. 
> This major mode this major mode, in addition to font-lock for PHP implements the following features:
> * font-lock for html, javascript, css and phpdoc.
> * six different indentation styles (PSR, PEAR, Zend, Drupal, Wordpress, Symfony).
> * Imenu
> * Flymake
> * Which-function
> * a helper function to simplify the installation of parsers, in versions used to develop major-mode
> * PHP built-in server support
> * Shell interaction: execute PHP code in an inferior PHP process.

Thanks, I added Stefan, Philip and Yuan to the discussion, in case they have
comments.

> +---
> +*** New major mode 'php-ts-mode'.
> +A major mode based on the tree-sitter library for editing

This seems to be an incomplete sentence.

Also, I think we should add PHP to the list of modes in the "Program
Modes" node of the Emacs user manual.

> +(defun php-ts-mode-install-parsers ()
> +  "Install all the required treesitter parser.
                                          ^^^^^^
"parsers", in plural.

> +`php-ts-mode--language-source-alist' define which parsers to install."
                                        ^^^^^^
"defines".

> +(defcustom php-ts-mode-indent-offset 4
> +  "Number of spaces for each indentation step (default) in `php-ts-mode'."
                ^^^^^^
"columns", I guess?

And what does "(default)" mean here?

> +(defcustom php-ts-mode-js-css-indent-offset html-ts-mode-indent-offset
> +  "JavaScript and CSS indent spaces related to the <script> and <style> HTML tags.
> +By default, the value is the same as `html-ts-mode-indent-offset'"
                                                                    ^
Period missing there at the end of the sentence.

> +(defcustom php-ts-mode-php-executable (or (executable-find "php") "/usr/bin/php")
> +  "The location of PHP executable."
> +  :tag "PHP Executable"
> +  :version "30.1"
> +  :type 'string
           ^^^^^^^
Should this be 'file instead?

> +(defcustom php-ts-mode-php-config nil
> +  "The location of php.ini file.
> +If nil the default one is used to run the embedded webserver or
> +inferior PHP process."
> +  :tag "PHP Init file"
> +  :version "30.1"
> +  :type 'string

Likewise here.

> +(defcustom php-ts-mode-ws-document-root nil
> +  "The root of the documents that the PHP built-in webserver will serve.
> +If nil `php-ts-mode-run-php-webserver' will ask you for the document root."
> +  :tag "PHP built-in web server document root"
> +  :version "30.1"
> +  :type 'string

And this one perhaps should be 'directory?

> +(defun php-ts-mode--array-element-heuristic (node parent bol &rest _)
> +  "Return of the position of the first element of the array.

The "of" part should be deleted here, I think.

> +(defun php-ts-mode-run-php-webserver (port hostname document-root
> +					   &optional router-script num-of-workers)
> +  "Run the PHP Built-in web-server on a specified PORT.

This should mention the mandatory arguments.  How about

  Run PHP built-in web server on HOSTNAME:PORT to serve DOCUMENT-ROOT.

> +PORT: Port number of built-in web server, default `php-ts-mode-ws-port'.
> +If a default value is nil, the value is prompted.

Please avoid passive voice as much as possible.  In this case, I'd use

  Prompt for the port if the default value is nil.

Btw, it will prompt also if the value of the argument is nil, right?
So the above should say that as well.

> +HOSTNAME: Hostname or IP address of Built-in web server,
> +default `php-ts-mode-ws-hostname'.  If a default value is nil,
> +the value is prompted.
> +DOCUMENT-ROOT: Path to Document root, default `php-ts-mode-ws-document-root'.
> +If a default value is nil, the value is prompted.

Same comments for these two arguments.

> +When called with \\[universal-argument] it requires PORT,
> +HOSTNAME, DOCUMENT-ROOT and ROUTER-SCRIPT."

Our style of saying that is like this:

  Interactively, when invoked with prefix argument, always prompt
  for PORT, HOSTNAME, DOCUMENT-ROOT and ROUTER-SCRIPT.

> +    (cond (num-of-workers (setenv "PHP_CLI_SERVER_WORKERS" num-of-workers))
> +	  (php-ts-mode-ws-workers
> +	   (setenv "PHP_CLI_SERVER_WORKERS" php-ts-mode-ws-workers)))

setenv modifies process-environment for the entire duration of this
Emacs session.  If we only want to affect the following invocation of
make-comint, then perhaps let-binding process-environment around the
call to make-comint is a better way?

> +(defun run-php (&optional cmd config)
> +  "Run a PHP interpreter as a inferior process.
                               ^
"an"

> +Argumens CMD an CONFIG, defaults to `php-ts-mode-php-executable'
                           ^^^^^^^^
"default", in plural.  Also, I think it is better to say "which
default", given the rest of the sentence.

> +If CONFIG is nil the intepreter run with the default php.ini.
                                   ^^^
"runs", singular.

> +if `php-ts-mode-php-executable' is not defined the user is prompted."
   ^^                                             ^^^^^^^^^^^^^^^^^^^^
"If", capitalized.  And please use "prompt the user" to avoid the
passive voice.

> +(defun inferior-php-ts-mode-startup (cmd config)
> +  "Start an inferior PHP process.
> +CMD is the command to run, CONFIG, if not nil, is a php.ini file to use."

Again, please try to mention the mandatory arguments in the first
line of the doc string.  For example:

  Start an inferior PHP process with command CMD and init file CONFIG.