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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 70939 in the body.
You can then email your comments to 70939 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

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

From: Ankit Gadiya <ankit <at> argp.in>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Add commands to run unit tests in go-ts-mode
Date: Tue, 14 May 2024 19:34:48 +0530

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

Hello folks,

This patch adds two new commands for go-ts-mode to run unit test cases.

The go-ts-mode-test-package command can run all the tests under the package
of
the current buffer. I've tested it to work with both Go module packages and
non-module packages. This command is bound to C-c C-p in the go-ts-mode-map.

The go-ts-mode-test-function-at-point command runs the current test
function. If
region is active then it runs all the test functions under the region. I'm
attaching a sample_test.go file as well for reviewers to test the patch.
This
command is bound to C-c C-t in the go-ts-mode-map.

-- 
Ankit

[Message part 2 (text/html, inline)]

[sample_test.go (text/x-go, attachment)]

[0001-Add-commands-to-run-unit-tests-in-go-ts-mode.patch (text/x-patch, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Ankit Gadiya <ankit <at> argp.in>, Randy Taylor <dev <at> rjt.dev>
Cc: 70939 <at> debbugs.gnu.org
Subject: Re: bug#70939: [PATCH] Add commands to run unit tests in go-ts-mode
Date: Tue, 14 May 2024 19:52:21 +0300

> Date: Tue, 14 May 2024 19:34:48 +0530
> From:  Ankit Gadiya via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> Hello folks,
> 
> This patch adds two new commands for go-ts-mode to run unit test cases.
> 
> The go-ts-mode-test-package command can run all the tests under the package of
> the current buffer. I've tested it to work with both Go module packages and
> non-module packages. This command is bound to C-c C-p in the go-ts-mode-map.
> 
> The go-ts-mode-test-function-at-point command runs the current test function. If
> region is active then it runs all the test functions under the region. I'm
> attaching a sample_test.go file as well for reviewers to test the patch. This
> command is bound to C-c C-t in the go-ts-mode-map.

Thanks.

Randy, any comments about this?

Ankit, we'd need a copyright assignment from you to accept a large
contribution such as this one.  Would you like to start the assignment
paperwork at this time?  If yes, I will send you the form to fill and
the instructions to go with it.

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

From: Ankit Gadiya <ankit <at> argp.in>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Randy Taylor <dev <at> rjt.dev>, 70939 <at> debbugs.gnu.org
Subject: Re: bug#70939: [PATCH] Add commands to run unit tests in go-ts-mode
Date: Tue, 14 May 2024 22:54:22 +0530

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

>
> Ankit, we'd need a copyright assignment from you to accept a large
> contribution such as this one.  Would you like to start the assignment
> paperwork at this time?  If yes, I will send you the form to fill and
> the instructions to go with it.
>

Hello Eli, I've already sent the form to assign <at> gnu.org following the
instructions in the CONTRIBUTE file.

--
Ankit

[Message part 2 (text/html, inline)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Ankit Gadiya <ankit <at> argp.in>
Cc: dev <at> rjt.dev, 70939 <at> debbugs.gnu.org
Subject: Re: bug#70939: [PATCH] Add commands to run unit tests in go-ts-mode
Date: Tue, 14 May 2024 20:59:08 +0300

> From: Ankit Gadiya <ankit <at> argp.in>
> Date: Tue, 14 May 2024 22:54:22 +0530
> Cc: Randy Taylor <dev <at> rjt.dev>, 70939 <at> debbugs.gnu.org
> 
>  Ankit, we'd need a copyright assignment from you to accept a large
>  contribution such as this one.  Would you like to start the assignment
>  paperwork at this time?  If yes, I will send you the form to fill and
>  the instructions to go with it.
> 
> Hello Eli, I've already sent the form to assign <at> gnu.org following the
> instructions in the CONTRIBUTE file.

Great, thanks.

Message #17 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, 15 May 2024 02:36:15 +0000

On Tuesday, May 14th, 2024 at 10:04, Ankit Gadiya via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org> wrote:
> Hello folks,
> 
> This patch adds two new commands for go-ts-mode to run unit test cases.
> 
> The go-ts-mode-test-package command can run all the tests under the package of
> the current buffer. I've tested it to work with both Go module packages and
> non-module packages. This command is bound to C-c C-p in the go-ts-mode-map.
> 
> The go-ts-mode-test-function-at-point command runs the current test function. If
> region is active then it runs all the test functions under the region. I'm
> attaching a sample_test.go file as well for reviewers to test the patch. This
> command is bound to C-c C-t in the go-ts-mode-map.
> 
> --
> Ankit

Thanks for working on this, it would certainly be a nice addition.

This is going to need a commit log entry. See the "Commit messages" section
in the CONTRIBUTING file.

I'm undecided on the keybinds. I think I would prefer something like:
C-c t p
or
C-c C-t p
so we can keep test-related things together.

I haven't tried this out yet, but here are some comments (mostly nits)
after a quick look:

In NEWS, sentences should be separated by 2 spaces.

+The 'go-ts-mode-test-function-at-point' command runs unit test at
                                                     ^the
+point.

+This function respects `go-build-tags' buffer-local variable
                       ^the

+  "Compiles the tests matching REGEXP.
    ^Compile

+If the point is anywhere in the test function, that function will be
+tested.
^ run (keeps it consistent with the next sentence)

+  "Run all the unit tests under current package."
                                ^the

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

From: Ankit Gadiya <ankit <at> argp.in>
To: Randy Taylor <dev <at> rjt.dev>
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, 15 May 2024 10:25:55 +0530

> This is going to need a commit log entry. See the "Commit messages" section
> in the CONTRIBUTING file.

I missed it earlier, will add it.

> I'm undecided on the keybinds. I think I would prefer something like:
> C-c t p
> or
> C-c C-t p
> so we can keep test-related things together.

Keeping it under C-c C-t makes sense to me. How about this:

C-c C-t t - go-ts-mode-test-function-at-point
C-c C-t p - go-ts-mode-test-package

> I haven't tried this out yet, but here are some comments (mostly nits)
> after a quick look:
>
> In NEWS, sentences should be separated by 2 spaces.
>
> +The 'go-ts-mode-test-function-at-point' command runs unit test at
>                                                      ^the
> +point.
>
> +This function respects `go-build-tags' buffer-local variable
>                        ^the
>
> +  "Compiles the tests matching REGEXP.
>     ^Compile
>
> +If the point is anywhere in the test function, that function will be
> +tested.
> ^ run (keeps it consistent with the next sentence)
>
> +  "Run all the unit tests under current package."
>                                 ^the

Thanks, I'll update this along with keybinding changes.

-- 
Ankit

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Randy Taylor <dev <at> rjt.dev>
Cc: ankit <at> argp.in, 70939 <at> debbugs.gnu.org
Subject: Re: bug#70939: [PATCH] Add commands to run unit tests in go-ts-mode
Date: Wed, 15 May 2024 14:21:43 +0300

> Date: Wed, 15 May 2024 02:36:15 +0000
> From: Randy Taylor <dev <at> rjt.dev>
> Cc: 70939 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
> 
> I'm undecided on the keybinds. I think I would prefer something like:
> C-c t p
> or
> C-c C-t p
> so we can keep test-related things together.

In general, key bindings that begin with "C-c" and a letter are
reserved for users.  So the second alternative is preferred.

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

From: Randy Taylor <dev <at> rjt.dev>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: ankit <at> argp.in, 70939 <at> debbugs.gnu.org
Subject: Re: bug#70939: [PATCH] Add commands to run unit tests in go-ts-mode
Date: Thu, 16 May 2024 01:24:46 +0000

On Wednesday, May 15th, 2024 at 07:21, Eli Zaretskii <eliz <at> gnu.org> wrote:
> 
> 
> > Date: Wed, 15 May 2024 02:36:15 +0000
> 
> > From: Randy Taylor dev <at> rjt.dev
> > Cc: 70939 <at> debbugs.gnu.org, Eli Zaretskii eliz <at> gnu.org
> > 
> > I'm undecided on the keybinds. I think I would prefer something like:
> > C-c t p
> > or
> > C-c C-t p
> > so we can keep test-related things together.
> 
> 
> In general, key bindings that begin with "C-c" and a letter are
> reserved for users. So the second alternative is preferred.

Noted, thanks.

Message #29 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: Thu, 16 May 2024 02:32:42 +0000

On Wednesday, May 15th, 2024 at 00:55, Ankit Gadiya <ankit <at> argp.in> wrote:
> 
> 
> > This is going to need a commit log entry. See the "Commit messages" section
> 
> > in the CONTRIBUTING file.
> 
> 
> I missed it earlier, will add it.
> 
> > I'm undecided on the keybinds. I think I would prefer something like:
> > C-c t p
> > or
> > C-c C-t p
> > so we can keep test-related things together.
> 
> 
> Keeping it under C-c C-t makes sense to me. How about this:
> 
> C-c C-t t - go-ts-mode-test-function-at-point
> C-c C-t p - go-ts-mode-test-package

Sounds good.

I'm wondering If C-c C-t t should be f for "function" but let's leave it as t for now. I do like the t for test which is nice and simple. Decisions, decisions...

> 
> > I haven't tried this out yet, but here are some comments (mostly nits)
> > after a quick look:
> > 
> > In NEWS, sentences should be separated by 2 spaces.
> > 
> > +The 'go-ts-mode-test-function-at-point' command runs unit test at
> > ^the
> > +point.
> > 
> > +This function respects `go-build-tags' buffer-local variable
> > ^the
> > 
> > + "Compiles the tests matching REGEXP.
> > ^Compile
> > 
> > +If the point is anywhere in the test function, that function will be
> > +tested.
> > ^ run (keeps it consistent with the next sentence)
> > 
> > + "Run all the unit tests under current package."
> > ^the
> 
> 
> Thanks, I'll update this along with keybinding changes.

Great. I'll try to give this a try tomorrow.

Eli, is there a convention regarding local variables?
In the patch we have:
+  (if (local-variable-p 'go-build-tags)
+      (format "-tags %s" go-build-tags)
+    ""))

Should that local variable be prefixed with go-ts-mode, or is it fine as is?

> 
> --
> Ankit

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Randy Taylor <dev <at> rjt.dev>
Cc: ankit <at> argp.in, 70939 <at> debbugs.gnu.org
Subject: Re: bug#70939: [PATCH] Add commands to run unit tests in go-ts-mode
Date: Thu, 16 May 2024 11:27:20 +0300

> Date: Thu, 16 May 2024 02:32:42 +0000
> From: Randy Taylor <dev <at> rjt.dev>
> Cc: 70939 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
> 
> Eli, is there a convention regarding local variables?
> In the patch we have:
> +  (if (local-variable-p 'go-build-tags)
> +      (format "-tags %s" go-build-tags)
> +    ""))
> 
> Should that local variable be prefixed with go-ts-mode, or is it fine as is?

If the variable is specific to go-ts-mode, it should indeed be
prefixed with "go-ts-".  But I don't see this variable in
go-ts-mode.el nor in the patch (did I miss something?), and I don't
understand why the above condition insists on using only a
buffer-local value of the variable in the first place?  What's wrong
with having a global value for the variable?

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

From: Ankit Gadiya <ankit <at> argp.in>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Randy Taylor <dev <at> rjt.dev>, 70939 <at> debbugs.gnu.org
Subject: Re: bug#70939: [PATCH] Add commands to run unit tests in go-ts-mode
Date: Thu, 16 May 2024 20:33:31 +0530

> > Eli, is there a convention regarding local variables?
> > In the patch we have:
> > +  (if (local-variable-p 'go-build-tags)
> > +      (format "-tags %s" go-build-tags)
> > +    ""))
> >
> > Should that local variable be prefixed with go-ts-mode, or is it fine as is?
>
> If the variable is specific to go-ts-mode, it should indeed be
> prefixed with "go-ts-".  But I don't see this variable in
> go-ts-mode.el nor in the patch (did I miss something?), and I don't
> understand why the above condition insists on using only a
> buffer-local value of the variable in the first place?  What's wrong
> with having a global value for the variable?

I'm not yet familiar with Elisp-idioms but I'll explain my idea and why I used
the variable this way.

The build tags can be unique to the specific project and sometimes even per
go package. So I thought to make it buffer specific rather than a global
variable defined in the init.el. I'm using these changes on my machine and I'm
defining the build tags in the dir-locals file. This way the variable is always
available for the mode commands to access.

But now I'm thinking maybe it makes sense to add it as defvar-local under the
go-ts-mode? Please guide me with what will be the correct way to implement it.

-- 
Ankit

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Ankit Gadiya <ankit <at> argp.in>
Cc: dev <at> rjt.dev, 70939 <at> debbugs.gnu.org
Subject: Re: bug#70939: [PATCH] Add commands to run unit tests in go-ts-mode
Date: Thu, 16 May 2024 19:01:11 +0300

> From: Ankit Gadiya <ankit <at> argp.in>
> Date: Thu, 16 May 2024 20:33:31 +0530
> Cc: Randy Taylor <dev <at> rjt.dev>, 70939 <at> debbugs.gnu.org
> 
> > If the variable is specific to go-ts-mode, it should indeed be
> > prefixed with "go-ts-".  But I don't see this variable in
> > go-ts-mode.el nor in the patch (did I miss something?), and I don't
> > understand why the above condition insists on using only a
> > buffer-local value of the variable in the first place?  What's wrong
> > with having a global value for the variable?
> 
> I'm not yet familiar with Elisp-idioms but I'll explain my idea and why I used
> the variable this way.
> 
> The build tags can be unique to the specific project and sometimes even per
> go package. So I thought to make it buffer specific rather than a global
> variable defined in the init.el. I'm using these changes on my machine and I'm
> defining the build tags in the dir-locals file. This way the variable is always
> available for the mode commands to access.

It's okay to support buffer-local values, but using the value of the
variable will do that automatically; you don't have to verify it has a
local binding.  What I don't understand is why does your code _reject_
global bindings.  If some user wants to set up a global value for some
reason, or have a default global value and special local values in
some cases, why should we prevent that?

> But now I'm thinking maybe it makes sense to add it as defvar-local under the
> go-ts-mode? Please guide me with what will be the correct way to implement it.

Yes, defvar-local could be what we want.  But I first would like to
understand what is special about this variable that it should be
buffer-local.

Message #41 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: Fri, 17 May 2024 02:27:54 +0000

On Wednesday, May 15th, 2024 at 22:32, Randy Taylor <dev <at> rjt.dev> wrote:
> 
> Great. I'll try to give this a try tomorrow.

Hi Ankit,

I've played around a little bit and noticed an issue when using go-ts-mode-test-function-at-point:
- When there is no region selected, the test could run more than the function at point
  if any function names are the same but differ in suffixes (e.g. TestQuack and TestQuackMoo
  would both run if invoked inside just TestQuack). The same applies to when a region is
  selected. Everything within the region is run, and potentially stuff outside of it too.
  We need to make the regex strictly the functions we want to run with '^TestQuack$'.

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

From: Ankit Gadiya <ankit <at> argp.in>
To: Randy Taylor <dev <at> rjt.dev>
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: Sat, 18 May 2024 14:25:14 +0530

> I've played around a little bit and noticed an issue when using go-ts-mode-test-function-at-point:
> - When there is no region selected, the test could run more than the function at point
>   if any function names are the same but differ in suffixes (e.g. TestQuack and TestQuackMoo
>   would both run if invoked inside just TestQuack). The same applies to when a region is
>   selected. Everything within the region is run, and potentially stuff outside of it too.
>   We need to make the regex strictly the functions we want to run with '^TestQuack$'.

Thanks for trying out the patch. Yes, I now see how it can capture functions
with the same prefix. I will make the regexp stricter and update the
sample_test.go as well with common prefix examples.

--
Ankit

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

From: Ankit Gadiya <ankit <at> argp.in>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: dev <at> rjt.dev, 70939 <at> debbugs.gnu.org
Subject: Re: bug#70939: [PATCH] Add commands to run unit tests in go-ts-mode
Date: Sat, 18 May 2024 15:24:14 +0530

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

> It's okay to support buffer-local values, but using the value of the
> variable will do that automatically; you don't have to verify it has a
> local binding.  What I don't understand is why does your code _reject_
> global bindings.  If some user wants to set up a global value for some
> reason, or have a default global value and special local values in
> some cases, why should we prevent that?

You are absolutely right, now I realize that local-variable-p is overly
conservative. Since I didn't define the variable in the first place I had to do
the check and even then boundp would have been better.

> > But now I'm thinking maybe it makes sense to add it as defvar-local under the
> > go-ts-mode? Please guide me with what will be the correct way to implement it.
>
> Yes, defvar-local could be what we want.  But I first would like to
> understand what is special about this variable that it should be
> buffer-local.

With the improved understanding, I believe defcustom would be better for the
variable. Then I don't even need the the boundp check since the variable is
always present and it can be replaced by just the non-nil check instead.

As an improvement, I'm also changing the variable-type to a list of strings
instead of a single comma separated string.

I'm submitting an updated Patch with the following changes:
* Add commit log
* Fix the text formatting.
* Update the keybindings to use the C-c C-t prefix.
* Improve regexp matching to be more strict.
* Define =go-ts-mode-build-tags= variable in the module and use it in the test
  functions.

-- 
Ankit

[0001-Add-commands-to-run-unit-tests-in-go-ts-mode.patch (text/x-patch, attachment)]

[sample_test.go (text/x-go, attachment)]

Message #50 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: Sat, 25 May 2024 02:35:42 +0000

On Saturday, May 18th, 2024 at 05:54, Ankit Gadiya via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org> wrote:
> 
> [...]
> 
> I'm submitting an updated Patch with the following changes:
> * Add commit log
> * Fix the text formatting.
> * Update the keybindings to use the C-c C-t prefix.
> * Improve regexp matching to be more strict.
> * Define =go-ts-mode-build-tags= variable in the module and use it in the test
> functions.
> 
> --
> Ankit

Thanks.

Sorry for the delay in reviewing, I've been having internet troubles since last Friday. I'll take a look at this next week.

A few quick things I noticed on a glance:

+  (let* ((node (go-ts-mode--find-defun-at start))
+	 (name (treesit-defun-name node))

Indentation is off on the name line - looks like a TAB was used? Should only be spaces everywhere. Double check the rest is OK.

+region.  It is bound to 'C-c C-t' in 'go-ts-mode'.
                         ^ C-c C-t t

+package of the current buffer.  It is bound to 'C-c C-p' in 'go-ts-mode'.
                                                ^ C-c C-t p

+  "List of go build tags for the test commands."
            ^ Go

+  "Return a list with names of all defuns in the range."
We should probably say what the range actually is (START to END) - not sure if we have a convention for that wording already.

Message #53 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: Tue, 28 May 2024 02:30:18 +0000

On Friday, May 24th, 2024 at 22:35, Randy Taylor <dev <at> rjt.dev> wrote:
> 
> On Saturday, May 18th, 2024 at 05:54, Ankit Gadiya via "Bug reports for GNU Emacs, the Swiss army knife of text editors" bug-gnu-emacs <at> gnu.org wrote:
> 
> > [...]
> > 
> > I'm submitting an updated Patch with the following changes:
> > * Add commit log
> > * Fix the text formatting.
> > * Update the keybindings to use the C-c C-t prefix.
> > * Improve regexp matching to be more strict.
> > * Define =go-ts-mode-build-tags= variable in the module and use it in the test
> > functions.
> > 
> > --
> > Ankit
> 
> 
> Thanks.
> 
> Sorry for the delay in reviewing, I've been having internet troubles since last Friday. I'll take a look at this next week.
> 
> A few quick things I noticed on a glance:
> 
> + (let* ((node (go-ts-mode--find-defun-at start))
> + (name (treesit-defun-name node))
> 
> Indentation is off on the name line - looks like a TAB was used? Should only be spaces everywhere. Double check the rest is OK.
> 
> +region. It is bound to 'C-c C-t' in 'go-ts-mode'.
> ^ C-c C-t t
> 
> +package of the current buffer. It is bound to 'C-c C-p' in 'go-ts-mode'.
> ^ C-c C-t p
> 
> + "List of go build tags for the test commands."
> ^ Go
> 
> + "Return a list with names of all defuns in the range."
> We should probably say what the range actually is (START to END) - not sure if we have a convention for that wording already.

A few more nits:
+  "Return compile flag for build tags.
          ^ the

+This function respects `go-ts-mode-build-tags' variable for specifying
                       ^ the

+  "Return a list with names of all defuns in the range."
                      ^ the

Indentation is also off in go-ts-mode-test-function-at-point.

When we run C-c C-t t outside of a function, we get:
go test -v  -run '^nil$'
Should we maybe not bother running anything at all? What do you think?
Do we know how other packages behave under similar circumstances?

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

From: Ankit Gadiya <ankit <at> argp.in>
To: Randy Taylor <dev <at> rjt.dev>
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, 29 May 2024 01:28:38 +0530

> > Sorry for the delay in reviewing, I've been having internet troubles since last Friday. I'll take a look at this next week.

No issues, appreciate the review.

> > A few quick things I noticed on a glance:
> >
> > + (let* ((node (go-ts-mode--find-defun-at start))
> > + (name (treesit-defun-name node))
> >
> > Indentation is off on the name line - looks like a TAB was used? Should only be spaces everywhere. Double check the rest is OK.
> >
> > +region. It is bound to 'C-c C-t' in 'go-ts-mode'.
> > ^ C-c C-t t
> >
> > +package of the current buffer. It is bound to 'C-c C-p' in 'go-ts-mode'.
> > ^ C-c C-t p
> >
> > + "List of go build tags for the test commands."
> > ^ Go
> >
> > + "Return a list with names of all defuns in the range."
> > We should probably say what the range actually is (START to END) - not sure if we have a convention for that wording already.
>
> A few more nits:
> +  "Return compile flag for build tags.
>           ^ the
>
> +This function respects `go-ts-mode-build-tags' variable for specifying
>                        ^ the
>
> +  "Return a list with names of all defuns in the range."
>                       ^ the
>
> Indentation is also off in go-ts-mode-test-function-at-point.

Thanks. I learned about whitespace-mode and checkdoc. I'll be running my changes
through it before sending the path now. Please let me know if there are any
other checks I can do in the future.

> When we run C-c C-t t outside of a function, we get:
> go test -v  -run '^nil$'
> Should we maybe not bother running anything at all? What do you think?
> Do we know how other packages behave under similar circumstances?

I'll check the packages and find out how it is handled elsewhere. One point of
reference can be Doom Emacs configuration. It uses the
re-search-(backward|forward) functions that throw an error if no match is found.

I'm also planning to add a third function to run all the tests in the current
file using the buffer-beginning and buffer-end as the range. I'll submit it in
the next patch along with the suggested fixes.

-- 
Ankit

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

From: Ankit Gadiya <ankit <at> argp.in>
To: Randy Taylor <dev <at> rjt.dev>
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, 19 Jun 2024 23:47:29 +0530

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

Apologies for the delay in response.

> > > A few quick things I noticed on a glance:
> > >
> > > + (let* ((node (go-ts-mode--find-defun-at start))
> > > + (name (treesit-defun-name node))
> > >
> > > Indentation is off on the name line - looks like a TAB was used? Should only be spaces everywhere. Double check the rest is OK.
> > >
> > > +region. It is bound to 'C-c C-t' in 'go-ts-mode'.
> > > ^ C-c C-t t
> > >
> > > +package of the current buffer. It is bound to 'C-c C-p' in 'go-ts-mode'.
> > > ^ C-c C-t p
> > >
> > > + "List of go build tags for the test commands."
> > > ^ Go
> > >
> > > + "Return a list with names of all defuns in the range."
> > > We should probably say what the range actually is (START to END) - not sure if we have a convention for that wording already.
> >
> > A few more nits:
> > +  "Return compile flag for build tags.
> >           ^ the
> >
> > +This function respects `go-ts-mode-build-tags' variable for specifying
> >                        ^ the
> >
> > +  "Return a list with names of all defuns in the range."
> >                       ^ the
> >
> > Indentation is also off in go-ts-mode-test-function-at-point.
>
> Thanks. I learned about whitespace-mode and checkdoc. I'll be running my changes
> through it before sending the path now. Please let me know if there are any
> other checks I can do in the future.

I've incorporated these suggestions in my updated patch.

> > When we run C-c C-t t outside of a function, we get:
> > go test -v  -run '^nil$'
> > Should we maybe not bother running anything at all? What do you think?
> > Do we know how other packages behave under similar circumstances?
>
> I'll check the packages and find out how it is handled elsewhere. One point of
> reference can be Doom Emacs configuration. It uses the
> re-search-(backward|forward) functions that throw an error if no match is found.

I've updated the functions to now raise an error if no function is found at
point or under the region.

> I'm also planning to add a third function to run all the tests in the current
> file using the buffer-beginning and buffer-end as the range. I'll submit it in
> the next patch along with the suggested fixes.

I've added the go-ts-mode-test-file function as well that runs all the unit
tests in the current file.

--
Ankit

[0001-Add-commands-to-run-unit-tests-in-go-ts-mode.patch (text/x-patch, attachment)]

Message #62 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: Fri, 21 Jun 2024 02:40:12 +0000

On Wednesday, June 19th, 2024 at 14:17, Ankit Gadiya via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org> wrote:
> 
> 
> Apologies for the delay in response.
> 
> > > > A few quick things I noticed on a glance:
> > > > 
> > > > + (let* ((node (go-ts-mode--find-defun-at start))
> > > > + (name (treesit-defun-name node))
> > > > 
> > > > Indentation is off on the name line - looks like a TAB was used? Should only be spaces everywhere. Double check the rest is OK.
> > > > 
> > > > +region. It is bound to 'C-c C-t' in 'go-ts-mode'.
> > > > ^ C-c C-t t
> > > > 
> > > > +package of the current buffer. It is bound to 'C-c C-p' in 'go-ts-mode'.
> > > > ^ C-c C-t p
> > > > 
> > > > + "List of go build tags for the test commands."
> > > > ^ Go
> > > > 
> > > > + "Return a list with names of all defuns in the range."
> > > > We should probably say what the range actually is (START to END) - not sure if we have a convention for that wording already.
> > > 
> > > A few more nits:
> > > + "Return compile flag for build tags.
> > > ^ the
> > > 
> > > +This function respects `go-ts-mode-build-tags' variable for specifying
> > > ^ the
> > > 
> > > + "Return a list with names of all defuns in the range."
> > > ^ the
> > > 
> > > Indentation is also off in go-ts-mode-test-function-at-point.
> > 
> > Thanks. I learned about whitespace-mode and checkdoc. I'll be running my changes
> > through it before sending the path now. Please let me know if there are any
> > other checks I can do in the future.
> 
> 
> I've incorporated these suggestions in my updated patch.
> 
> > > When we run C-c C-t t outside of a function, we get:
> > > go test -v -run '^nil$'
> > > Should we maybe not bother running anything at all? What do you think?
> > > Do we know how other packages behave under similar circumstances?
> > 
> > I'll check the packages and find out how it is handled elsewhere. One point of
> > reference can be Doom Emacs configuration. It uses the
> > re-search-(backward|forward) functions that throw an error if no match is found.
> 
> 
> I've updated the functions to now raise an error if no function is found at
> point or under the region.
> 
> > I'm also planning to add a third function to run all the tests in the current
> > file using the buffer-beginning and buffer-end as the range. I'll submit it in
> > the next patch along with the suggested fixes.
> 
> 
> I've added the go-ts-mode-test-file function as well that runs all the unit
> tests in the current file.
> 
> --
> Ankit

When running go-ts-mode-test-file, it seems to match things that aren't functions like interfaces. I think we should only be matching functions, and more specifically, shouldn't we only be matching functions starting with "Test"?
We could perhaps extend the error checking to include that as well?

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?

+*** New unit test commands.
+Two new commands are now available to run unit tests.
Three?

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?

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

From: Ankit Gadiya <ankit <at> argp.in>
To: Randy Taylor <dev <at> rjt.dev>
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: Sun, 23 Jun 2024 20:16:23 +0530

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

> When running go-ts-mode-test-file, it seems to match things that aren't functions like interfaces. I think we should only be matching functions, and more specifically, shouldn't we only be matching functions starting with "Test"?
> We could perhaps extend the error checking to include that as well?

I've updated the patch to include a "function_declaration" and "Test" prefix
check. I also did a minor refactoring to avoid special handling in the case when
the region is not active. I'm also attaching the updated sample file with more
scenarios for testing.

>
> 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?
>
> +*** New unit test commands.
> +Two new commands are now available to run unit tests.
> Three?
>
> 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 :).

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?

--
Ankit

[0001-Add-commands-to-run-unit-tests-in-go-ts-mode.patch (text/x-patch, attachment)]

[sample_test.go (text/x-go, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Randy Taylor <dev <at> rjt.dev>
Cc: ankit <at> argp.in, 70939 <at> debbugs.gnu.org
Subject: Re: bug#70939: [PATCH] Add commands to run unit tests in go-ts-mode
Date: Sun, 23 Jun 2024 17:56:02 +0300

> Date: Fri, 21 Jun 2024 02:40:12 +0000
> From: Randy Taylor <dev <at> rjt.dev>
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 70939 <at> debbugs.gnu.org
> 
> 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?

Yes, the commit log doesn't need to say again what is already in NEWS,
it can be shorter.

> 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'd use "this" instead of "current".

Message #71 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, 26 Jun 2024 02:26:35 +0000

On Sunday, June 23rd, 2024 at 10:46, Ankit Gadiya <ankit <at> argp.in> wrote:
> 
> 
> > When running go-ts-mode-test-file, it seems to match things that aren't functions like interfaces. I think we should only be matching functions, and more specifically, shouldn't we only be matching functions starting with "Test"?
> 
> > We could perhaps extend the error checking to include that as well?
> 
> 
> I've updated the patch to include a "function_declaration" and "Test" prefix
> check. I also did a minor refactoring to avoid special handling in the case when
> the region is not active. I'm also attaching the updated sample file with more
> scenarios for testing.

Thanks, the changes look good to me.

> 
> > 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?
> > 
> > +*** New unit test commands.
> > +Two new commands are now available to run unit tests.
> > Three?

This still needs to be updated.

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?

Also, the indentation looks off in go-ts-mode-test-function-at-point (2 extra spaces methinks).

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

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

> 
> --
> Ankit

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Randy Taylor <dev <at> rjt.dev>
Cc: ankit <at> argp.in, 70939 <at> debbugs.gnu.org
Subject: Re: bug#70939: [PATCH] Add commands to run unit tests in go-ts-mode
Date: Wed, 26 Jun 2024 14:27:28 +0300

> Date: Wed, 26 Jun 2024 02:26:35 +0000
> From: Randy Taylor <dev <at> rjt.dev>
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 70939 <at> debbugs.gnu.org
> 
> > 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 don't think it's urgent to have this on the release branch, is it?
If it isn't urgent, then I prefer master.

Thanks.

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

From: Randy Taylor <dev <at> rjt.dev>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: ankit <at> argp.in, 70939 <at> debbugs.gnu.org
Subject: Re: bug#70939: [PATCH] Add commands to run unit tests in go-ts-mode
Date: Wed, 26 Jun 2024 12:31:21 +0000

On Wednesday, June 26th, 2024 at 07:27, Eli Zaretskii <eliz <at> gnu.org> wrote:
> 
> 
> > Date: Wed, 26 Jun 2024 02:26:35 +0000
> 
> > From: Randy Taylor dev <at> rjt.dev
> > Cc: Eli Zaretskii eliz <at> gnu.org, 70939 <at> debbugs.gnu.org
> > 
> > > 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 don't think it's urgent to have this on the release branch, is it?
> If it isn't urgent, then I prefer master.
> 
> Thanks.

Not urgent at all. Master it is, thanks.

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

From: Ankit Gadiya <ankit <at> argp.in>
To: Randy Taylor <dev <at> rjt.dev>
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: Sun, 7 Jul 2024 01:14:01 +0530

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

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

[0001-Add-commands-to-run-unit-tests-in-go-ts-mode.patch (application/octet-stream, attachment)]

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Ankit Gadiya <ankit <at> argp.in>, Randy Taylor <dev <at> rjt.dev>
Cc: Dmitry Gutov <dmitry <at> gutov.dev>, 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: Sat, 6 Jul 2024 15:08:47 -0700

Ankit Gadiya via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs <at> gnu.org> writes:

> +*** New unit test commands.
> +Three new commands are now available to run unit tests.
> +
> +The 'go-ts-mode-test-function-at-point' command runs the unit test at
> +point.  If a region is active, it runs all the unit tests under the
> +region.  It is bound to 'C-c C-t t' in 'go-ts-mode'.
> +
> +The 'go-ts-mode-test-this-file' command runs all unit tests in the current
> +file. It is bound to 'C-c C-t f' in 'go-ts-mode'.
> +
> +The 'go-ts-mode-test-this-package' command runs all unit tests under the
> +package of the current buffer.  It is bound to 'C-c C-t p' in 'go-ts-mode'.
> +
> +The 'go-ts-mode-build-tags' variable is available to set a list of build
> +tags for the test commands.

BTW, eventually I think that we want a general command for this in
project.el, e.g.  "project-run-tests" or something like that.

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

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: Stefan Kangas <stefankangas <at> gmail.com>, Ankit Gadiya <ankit <at> argp.in>,
 Randy Taylor <dev <at> rjt.dev>
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: Sun, 7 Jul 2024 01:30:33 +0300

On 07/07/2024 01:08, Stefan Kangas wrote:
> BTW, eventually I think that we want a general command for this in
> project.el, e.g.  "project-run-tests" or something like that.

Some command (or even a set of commands) would indeed be useful. I'm not 
sure it should be tied to 'project', though - IME those commands usually 
only need to find the test root, and that one does not always match the 
project root.

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

From: Ankit Gadiya <ankit <at> argp.in>
To: Dmitry Gutov <dmitry <at> gutov.dev>
Cc: Randy Taylor <dev <at> rjt.dev>, Eli Zaretskii <eliz <at> gnu.org>,
 Stefan Kangas <stefankangas <at> gmail.com>, 70939 <at> debbugs.gnu.org
Subject: Re: bug#70939: [PATCH] Add commands to run unit tests in go-ts-mode
Date: Sun, 7 Jul 2024 12:56:51 +0530

On Sun, 7 Jul 2024 at 04:00, Dmitry Gutov <dmitry <at> gutov.dev> wrote:
>
> On 07/07/2024 01:08, Stefan Kangas wrote:
> > BTW, eventually I think that we want a general command for this in
> > project.el, e.g.  "project-run-tests" or something like that.
>
> Some command (or even a set of commands) would indeed be useful. I'm not
> sure it should be tied to 'project', though - IME those commands usually
> only need to find the test root, and that one does not always match the
> project root.

It would be good to have a separate module similar to compile.el. Then
mode-specific implementation can register test functions. The project.el
command(s) can then be added to run on project-root.

-- 
Ankit

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.

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

From: Ankit Gadiya <ankit <at> argp.in>
To: Randy Taylor <dev <at> rjt.dev>
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: Thu, 11 Jul 2024 13:03:32 +0530

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

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

Thanks, I'm sending the updated patch with these fixes.

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

Thank you for being patient through this review. After this interaction, I feel
encouraged to send more patches in the future.

-- 
Ankit

[0001-Add-commands-to-run-unit-tests-in-go-ts-mode.patch (text/x-patch, attachment)]

Message #100 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: Thu, 11 Jul 2024 14:21:59 +0000

On Thursday, July 11th, 2024 at 03:33, Ankit Gadiya <ankit <at> argp.in> wrote:
> 
> > 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-...).
> 
> 
> Thanks, I'm sending the updated patch with these fixes.

Thanks, looks good!

> 
> > 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.
> 
> 
> Thank you for being patient through this review. After this interaction, I feel
> encouraged to send more patches in the future.

Glad to hear that! Thanks for putting up with us :).

> 
> --
> Ankit

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Randy Taylor <dev <at> rjt.dev>
Cc: ankit <at> argp.in, 70939 <at> debbugs.gnu.org
Subject: Re: bug#70939: [PATCH] Add commands to run unit tests in go-ts-mode
Date: Fri, 12 Jul 2024 09:23:29 +0300

> Date: Thu, 11 Jul 2024 14:21:59 +0000
> From: Randy Taylor <dev <at> rjt.dev>
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 70939 <at> debbugs.gnu.org
> 
> > > 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.
> > 
> > 
> > Thank you for being patient through this review. After this interaction, I feel
> > encouraged to send more patches in the future.
> 
> Glad to hear that! Thanks for putting up with us :).

Thank you all.

Ankit, it looks like your legal paperwork of copyright assignment has
stalled?  The last message I have about it is from 2 months ago, where
you were sent the PDF of your assignment and asked to sign and email
it back to the FSF.  Your assignment is still not on file.  Did you
email the signed PDF back to the FSF clerk, and did you subsequently
receive your assignment counter-signed by the FSF?

If you didn't email the signed PDF, please do so now.  If you did,
please ping them saying you didn't receive the counter-signed document
and CC me.  If you received the counter-signed assignment document,
please tell me, because it means the FSF clerk somehow failed to
update the records with your assignment.

Thanks.

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

From: Ankit Gadiya <ankit <at> argp.in>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Randy Taylor <dev <at> rjt.dev>, 70939 <at> debbugs.gnu.org
Subject: Re: bug#70939: [PATCH] Add commands to run unit tests in go-ts-mode
Date: Fri, 12 Jul 2024 16:40:04 +0530

On Fri, 12 Jul 2024 at 11:53, Eli Zaretskii <eliz <at> gnu.org> wrote:
>
> > Date: Thu, 11 Jul 2024 14:21:59 +0000
> > From: Randy Taylor <dev <at> rjt.dev>
> > Cc: Eli Zaretskii <eliz <at> gnu.org>, 70939 <at> debbugs.gnu.org
> >
> > > > 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.
> > >
> > >
> > > Thank you for being patient through this review. After this interaction, I feel
> > > encouraged to send more patches in the future.
> >
> > Glad to hear that! Thanks for putting up with us :).
>
> Thank you all.
>
> Ankit, it looks like your legal paperwork of copyright assignment has
> stalled?  The last message I have about it is from 2 months ago, where
> you were sent the PDF of your assignment and asked to sign and email
> it back to the FSF.  Your assignment is still not on file.  Did you
> email the signed PDF back to the FSF clerk, and did you subsequently
> receive your assignment counter-signed by the FSF?
>
> If you didn't email the signed PDF, please do so now.  If you did,
> please ping them saying you didn't receive the counter-signed document
> and CC me.  If you received the counter-signed assignment document,
> please tell me, because it means the FSF clerk somehow failed to
> update the records with your assignment.
>
> Thanks.

Hey Eli, I emailed all the requirements for Copyright Assignments 2 months ago.
However, Craig informed me 5 days ago that he accidentally assigned the wrong
person so it stalled. He has fixed it and so I'm hoping the process will
complete soon.

-- 
Ankit

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Ankit Gadiya <ankit <at> argp.in>
Cc: dev <at> rjt.dev, 70939 <at> debbugs.gnu.org
Subject: Re: bug#70939: [PATCH] Add commands to run unit tests in go-ts-mode
Date: Fri, 12 Jul 2024 14:21:53 +0300

> From: Ankit Gadiya <ankit <at> argp.in>
> Date: Fri, 12 Jul 2024 16:40:04 +0530
> Cc: Randy Taylor <dev <at> rjt.dev>, 70939 <at> debbugs.gnu.org
> 
> Hey Eli, I emailed all the requirements for Copyright Assignments 2 months ago.
> However, Craig informed me 5 days ago that he accidentally assigned the wrong
> person so it stalled. He has fixed it and so I'm hoping the process will
> complete soon.

OK, thanks for the update.  If this doesn't come to completion in a
week or two, please ping Craig and CC me.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Ankit Gadiya <ankit <at> argp.in>
Cc: dev <at> rjt.dev, 70939-done <at> debbugs.gnu.org
Subject: Re: bug#70939: [PATCH] Add commands to run unit tests in go-ts-mode
Date: Sun, 21 Jul 2024 09:05:24 +0300

> From: Ankit Gadiya <ankit <at> argp.in>
> Date: Thu, 11 Jul 2024 13:03:32 +0530
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 70939 <at> debbugs.gnu.org
> 
> > 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-...).
> 
> Thanks, I'm sending the updated patch with these fixes.
> 
> > 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.
> 
> Thank you for being patient through this review. After this interaction, I feel
> encouraged to send more patches in the future.

Now installed on the master branch, and closing the bug.

Thanks.

Previous Next

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