GNU bug report logs - #71380
30.0.50; Submitting php-ts-mode, new major mode for php

Previous Next

Full log

View this message in rfc822 format

From: Vincenzo Pupillo <v.pupillo <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 71380 <at> debbugs.gnu.org
Subject: bug#71380: 30.0.50; Submitting php-ts-mode, new major mode for php
Date: Fri, 07 Jun 2024 12:45:05 +0200

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

Hi Eli, Thank you.

In data giovedì 6 giugno 2024 08:58:11 CEST, hai scritto:
> > From: Vincenzo Pupillo <v.pupillo <at> gmail.com>
> > 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.
Done

> 
> 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.
>
Done 
> > +`php-ts-mode--language-source-alist' define which parsers to install."
> 
>                                         ^^^^^^
> "defines".
> 
Done
> > +(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?
> 
The comment is the same as c-ts-mode-indent-offset except for “(default)” , 
which I deleted

> > +(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.
> 
Done 
> > +(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?
> 
Done

> > +(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.
>
Done
 
> > +(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?
> 
Done

> > +(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.
>
I'm not sure how to explain it. Different indentation styles indent the 
elements of an array differently when written on multiple rows. For example.
in PSR2 it is like this:
$a = array("a" => 1,
     "b" => 2,
     "c" => 3);
while with Zend it is like this:
$a = array("a" => 1,
           "b" => 2,
           "c" => 3);
What do you suggest?

> > +(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.
> 
Sorry, an error while merge my branch. all parameters are optional.

> > +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.
Yes, done.

> 
> > +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.
>
Ok, done.
 
> > +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.
> 
Done

> > +    (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?
> 
Yes is better. I rewrote as a let variable:
(process-environment
          (cons (cond
                 (num-of-workers (format "PHP_CLI_SERVER_WORKERS=%d" num-of-
workers))
                 (php-ts-mode-ws-workers (format "PHP_CLI_SERVER_WORKERS=%d" 
php-ts-mode-ws-workers)))
                process-environment))

PHP accepts that environment variable only if > 1, 
otherwise it issues in warning.


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

> > +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.
> 
Done

> > +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

Done


As I wrote to Andrea, the compilation warning seems to be caused 
by html-ts-mode. I have prepared another patch that fixes the problem.

Thank you.

Vincenzo

[0001-Add-php-ts-mode.patch (text/x-patch, attachment)]

[0001-Do-not-run-treesit-parser-create-if-HTML-parser-is-u.patch (text/x-patch, attachment)]

Previous Next

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