unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
From: Eli Zaretskii <eliz@gnu.org>
To: "Йордан Миладинов" <yordanm@proton.me>
Cc: dev@rjt.dev, casouri@gmail.com, 63708@debbugs.gnu.org
Subject: bug#63708: 29.0.60; rust-ts-mode not properly handling apostrophe
Date: Mon, 29 May 2023 14:25:50 +0300	[thread overview]
Message-ID: <83wn0rgz0h.fsf@gnu.org> (raw)
In-Reply-To: <Py-los7D7v-S2yEpw5m5oNbVRJ-eGPk9a4aBPk8iOnNAeKyHFvAWnSTG8oYfJDP_IFml65paJQvJs-Br-4E_tpadeTN72OFwQ-XFopjbrNI=@proton.me> (message from Йордан Миладинов on Sun, 28 May 2023 19:52:38 +0000)

> Date: Sun, 28 May 2023 19:52:38 +0000
> From: Йордан Миладинов <yordanm@proton.me>
> Cc: Eli Zaretskii <eliz@gnu.org>, 63708@debbugs.gnu.org, casouri@gmail.com
> 
> And here's the patch...

Thanks, a few comments below:

> * lisp/progmodes/rust-ts-mode.el (rust-ts-mode--syntax-propertize)
> 
>   Treat apostrophes as strings if used to define character literals.
> 
>   Treat LT and GT as pairs if used to define type parameters (formerly
>   they were treated as pairs only for type arguments).
> 
> * lisp/progmodes/go-ts-mode.el (go-ts-mode)
> 
>   Treat apostrophes as strings if used to define rune literals.

We don't leave empty lines between the name of the function and the
description of the change.  Like this:

* lisp/progmodes/rust-ts-mode.el (rust-ts-mode--syntax-propertize):
Treat apostrophes as strings if used to define character literals.
Treat LT and GT as pairs if used to define type parameters
(formerly they were treated as pairs only for type arguments).

* lisp/progmodes/go-ts-mode.el (go-ts-mode): Treat apostrophes as
strings if used to define rune literals.

Also note: no indentation, and a colon before the description.

> +(defun go-ts-mode--syntax-propertize (beg end)
> +    "Apply syntax properties to various special characters with
> +contextual meaning between BEG and END.

The first line of a doc string should be a single full sentence, and
should not exceed 78 characters.

>  (defun rust-ts-mode--syntax-propertize (beg end)
> -  "Apply syntax text property to template delimiters between BEG and END.
> +  "Apply syntax properties to various special characters with
> +contextual meaning between BEG and END.

Same here.

> +This function scans the changed RANGES for these special
> +characters and applies the appropriate text properties to reflect
> +their syntactical meaning."

This uses "RANGES", upper-case, which is our convention for arguments,
but there's no argument RANGES in this function.  (Yes, I know that
the original doc string also had this problem.)

>    (while (re-search-forward (rx (or "<" ">")) end t)
> -    (pcase (treesit-node-type
> -            (treesit-node-parent
> -             (treesit-node-at (match-beginning 0))))
> -      ("type_arguments"
> -       (put-text-property (match-beginning 0)
> -                          (match-end 0)
> -                          'syntax-table
> -                          (pcase (char-before)
> -                            (?< '(4 . ?>))
> -                            (?> '(5 . ?<))))))))
> +    (let ((parent-type (treesit-node-type
> +                        (treesit-node-parent
> +                         (treesit-node-at (match-beginning 0))))))
> +      (when (or (string-equal parent-type "type_arguments")
> +                (string-equal parent-type "type_parameters"))
> +        (put-text-property (match-beginning 0)
> +                           (match-end 0)
> +                           'syntax-table
> +                           (pcase (char-before)
> +                             (?< '(4 . ?>))
> +                             (?> '(5 . ?<))))))))

Here, AFAIU you needed to add a single line, but instead you rewrote a
'pcase' as a 'when', and got a much larger change.  Was that really
needed?

I also don't understand how a bug for go-ts-mode led to a change that
also affects rust-ts-mode.  Can you explain?

Finally, and most importantly: I don't see a copyright assignment from
you on file, which means we can only accept a small contribution from
you, less than 15 non-trivial lines of changes.  This patch is
significantly larger.  So we can only accept it if you start your
legal paperwork now, and only after the paperwork is finished.  Would
you like to start the papers rolling at this time?  If yes, I will
send you the form to fill.

Thanks again for working on this.





  reply	other threads:[~2023-05-29 11:25 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-05-24 21:40 bug#63708: 29.0.60; rust-ts-mode not properly handling apostrophe yordanm--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-24 22:05 ` bug#63709: " yordanm--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-25  5:28   ` Eli Zaretskii
2023-05-25  5:31 ` bug#63708: " Eli Zaretskii
2023-05-25 12:52   ` Randy Taylor
2023-05-25 13:13     ` Eli Zaretskii
2023-05-26  2:21       ` Randy Taylor
2023-05-26 19:14         ` yordanm--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-27  3:00           ` Randy Taylor
2023-05-28 19:51             ` yordanm--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-28 19:52               ` yordanm--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-29 11:25                 ` Eli Zaretskii [this message]
2023-05-29 13:34                   ` Randy Taylor
2023-05-29  1:29               ` Randy Taylor
2023-05-29 12:13                 ` Eli Zaretskii
2023-05-29 12:18                   ` Randy Taylor
2023-05-29 13:13                     ` Eli Zaretskii
2023-05-29 13:20                       ` Randy Taylor
2023-05-29 14:13                   ` yordanm--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-29 14:27                     ` Eli Zaretskii
2023-05-29 14:54                       ` Йордан Миладинов
2023-05-29 14:59                         ` Eli Zaretskii
2023-05-30  0:14                         ` Randy Taylor
2023-05-30  9:44                           ` yordanm--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-06-03  8:35                             ` Eli Zaretskii
2023-05-30  9:10 ` Йордан Миладинов

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.gnu.org/software/emacs/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=83wn0rgz0h.fsf@gnu.org \
    --to=eliz@gnu.org \
    --cc=63708@debbugs.gnu.org \
    --cc=casouri@gmail.com \
    --cc=dev@rjt.dev \
    --cc=yordanm@proton.me \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).