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

Previous Next

Full log

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Vincenzo Pupillo <v.pupillo <at> gmail.com>
Cc: 71380 <at> debbugs.gnu.org
Subject: Re: bug#71380: 30.0.50; Submitting php-ts-mode, new major mode for php
Date: Thu, 06 Jun 2024 10:06:21 -0400

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

?

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

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

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

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

> +(derived-mode-add-parents 'php-ts-mode '(php-mode))

I'd move it next to the `define-derived-mode`.


        Stefan

Previous Next

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