GNU bug report logs - #70939
[PATCH] Add commands to run unit tests in go-ts-mode

Previous Next

Full log

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

From: Randy Taylor <dev <at> rjt.dev>
To: Ankit Gadiya <ankit <at> argp.in>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 70939 <at> debbugs.gnu.org
Subject: Re: bug#70939: [PATCH] Add commands to run unit tests in go-ts-mode
Date: Wed, 10 Jul 2024 23:43:37 +0000

On Saturday, July 6th, 2024 at 15:44, Ankit Gadiya <ankit <at> argp.in> wrote:
> 
> Apologies for the delay again.
> 
> > > > For the commit message, I'm not sure we need that paragraph especially when it's already described in the news. Eli what do you think?
> 
> 
> Thanks Randy and Eli, I've updated the commit message to be shorter now.
> 
> > > > +*** New unit test commands.
> > > > +Two new commands are now available to run unit tests.
> > > > Three?
> > 
> > This still needs to be updated.
> 
> 
> Thanks, I fixed it.
> 
> > A few more comments:
> > +(defun go-ts-mode--get-test-regexp-at-point ()
> > + "Return a regular expression for tests at point.
> > ^ the
> > 
> > Could go-ts-mode--get-test-regexp-at-point and go-ts-mode-test-file use if-let?
> 
> 
> I wasn't familiar with if-let, thanks for this. Incorporated it in
> both the functions.
> 
> > Also, the indentation looks off in go-ts-mode-test-function-at-point (2 extra spaces methinks).
> 
> 
> Fixed this, thanks.
> 
> > > > I'm also wondering if we should include "current" in the go-ts-mode-test-file and go-ts-mode-test-package function names. Maybe someone would expect that they would get prompted to select a file or package to test? Maybe I'm overthinking that :). Eli what do you think?
> > > 
> > > I'll wait for Eli to reply before incorporating the changes :).
> > 
> > And he chimed in - let's go with his suggestions.
> 
> 
> Updated both the function names.
> go-ts-mode-test-this-file
> go-ts-mode-test-this-package
> 
> > > Additionally, I also noticed that the emacs-30 branch is cut. I wanted
> > > to check if I
> > > need to rebase my patch onto master or emacs-30 branch?
> > 
> > I would guess master, but let's see what Eli says.
> 
> 
> I rebased my patch on the master now. Requesting a re-review.
> 
> Thanks
> 
> --
> Ankit

I only have a few comments about the commit message:

Three new commands are added in the go-ts-mode to run unit tests.
I would just drop this line altogether, personally.

(go-ts-mode-map): New map variable.
This should probably read something like Add new bindings.

(go-ts-mode-test-file): New function.
(go-ts-mode-test-package): New function.
These two need to be updated (...-test-this-...).

Everything else looks good to me. Thanks for working on this, Ankit.

Eli, if you have no further comments please install when you get a chance. Thanks in advance.

Previous Next

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