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

Previous Next

Full log

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

From: Vincenzo Pupillo <v.pupillo <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 71380 <at> debbugs.gnu.org
Subject: Re: bug#71380: 30.0.50; Submitting php-ts-mode, new major mode for php
Date: Fri, 07 Jun 2024 11:04:00 +0200

Hi Stefan

In data giovedì 6 giugno 2024 16:06:21 CEST, Stefan Monnier ha scritto:
> I have not read the whole code, but just noticed the things below:
> 
> > +  (named-let loop ((res nil)
> > +		   (buffers (buffer-list)))
> > +    (if (null buffers)
> > +	(mapc (lambda (b)
> > +		(with-current-buffer b
> > +		  (php-ts-mode-set-style val)))
> > +	      res)
> > +      (let ((buffer (car buffers)))
> > +	(with-current-buffer buffer
> > +	  (if (derived-mode-p 'php-ts-mode)
> > +	      (loop (append res (list buffer)) (cdr buffers))
> > +	    (loop res (cdr buffers))))))))
> 
> These `loop` calls are not in tail-position, so this will eat up the
> stack (because of the surrounding `with-current-buffer`).
> Also, I don't understand why you do it in such a complicated way.
> Isn't this better written:
> 
>     (dolist (buffer (buffer-list))
>       (with-current-buffer buffer
>         (when (derived-mode-p 'php-ts-mode)
>           (php-ts-mode-set-style val))))
> 
> ?
Yes is better. The code above is a copy of c-ts-mode--indent-style-setter. 
It seemed too complicated to me too, but since it had been used there 
I thought there was some reason.

> 
> > +(defcustom php-ts-mode-indent-style 'psr2
> > +  "Style used for indentation.
> > +The selected style could be one of:
> > +`PSR-2/PSR-12' - use PSR standards (PSR-2, PSR-12), thi is the default.
> > +`PEAR' - use coding styles preferred for PEAR code and modules.
> > +`Drupal' - use coding styles preferred for working with Drupal projects.
> > +`WordPress' - use coding styles preferred for working with WordPress projects.
> > +`Symfony' - use coding styles preferred for working with Symfony projects.
> > +`Zend' - use coding styles preferred for working with Zend projects.
> > +
> > +If one of the supplied styles doesn't suffice, a function could be
> > +set instead.  This function is expected return a list that
> > +follows the form of `treesit-simple-indent-rules'."
> > +  :tag "PHP indent style"
> > +  :version "30.1"
> > +  :type '(choice (const :tag "PSR-2/PSR-12" psr2)
> > +		 (const :tag "PEAR" pear)
> > +		 (const :tag "Drupal" drupal)
> > +		 (const :tag "WordPress" wordpress)
> > +		 (const :tag "Symfony" symfony)
> > +		 (const :tag "Zend" zend)
> > +		 (function :tag "A function for user customized style" ignore))
> > +  :set #'php-ts-mode--indent-style-setter
> > +  :safe 'c-ts-indent-style-safep)
> 
> The :safe arg is also a function, so please #' it as well.
> 
Done

> > +(defvar php-ts-mode--syntax-table
> > +  (let ((table (make-syntax-table)))
> > +    ;; Taken from the cc-langs version
> 
> Does this mean it comes from "the cc-mode-based `php-mode.el`" or from
> `cc-langs.el` (and if so, which part, exactly)?
> 
> > +;; taken from c-ts-mode
> [...]
> > +;; taken from c-ts-mode
> 
> Are these literal copies?
> Maybe we should consolidate the code with that of `c-ts-mode` to avoid
> the code duplication?
> 
Yes, the first part is a literal copy of c-ts-mode--syntax-table. 
java-ts-mode does exactly the same thing, so I thought it best 
to avoid depending on c-ts-mode--syntax-table.


> > +  (cond
> > +   ((equal comment-start "/*") (setq-local comment-end "*/"))
> > +   ((equal comment-start "//") (setq-local comment-end ""))
> > +   ((equal comment-start "#") (setq-local comment-end ""))
> > +   ((equal comment-start "/**") (setq-local comment-end "*/")))
> > +  (setq mode-name (concat "PHP" (string-trim-right comment-start)))
> > +  (force-mode-line-update))
> 
> Is `comment-start` important enough to merit being part of the mode name?
> 

Sorry. I didn't understand.
Could you please clarify?

> > +(define-derived-mode php-ts-mode prog-mode "PHP"
> > +  "Major mode for editing PHP, powered by tree-sitter.
> > +
> > +\\{php-ts-mode-map}"
> 
> \\{php-ts-mode-map} is not necessary because `define-derived-mode` adds
> for you anyway.
> 
Ok, done.

> > +(derived-mode-add-parents 'php-ts-mode '(php-mode))
> 
> I'd move it next to the `define-derived-mode`.
> 
> 
Ok, done.
>         Stefan
> 
> 
Thank you Stefan.

Vincenzo

Previous Next

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