Re: [GSOC][PATCH] userdiff: Add JavaScript function patterns

From: Christian Couder <christian.couder@gmail.com>
Date: Mar 2, 2024, 10:28 UTC
Message-ID: <CAP8UFD1sAYORc_q20t4Y28mSjNa0BZ69XQ-hGFsNTzbThsG1KQ@mail.gmail.com>
In-Reply-To: <20240301074048.188835-1-sergiusnyah@gmail.com>

The subject should be "userdiff: add JavaScript function patterns" so with "add" instead of "Add". See the SubmittingPatches document which contains:

"The title sentence after the "area:" prefix omits the full stop at the end, and its first word is not capitalized (the omission of capitalization applies only to the word after the "area:" prefix of the title) unless there is a reason to capitalize it other than because it is the first word in the sentence."

On Fri, Mar 1, 2024 at 8:40 AM Sergius Nyah <sergiusnyah@gmail.com> wrote:

>
> This commit adds a patterns used to match JavaScript functions.

It should be either "add patterns" or "add a pattern".

Also instead of "This commit" please use the imperative mood or "Let's". See the SubmittingPatches document which contains:

"Describe your changes in imperative mood, e.g. "make xyzzy do frotz" instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy to do frotz", as if you are giving orders to the codebase to change its behavior."

Show 8 quoted lines

> It now correctly identifies function declarations, function expressions,
> and functions defined inside blocks. Add test for corresponding change in userdiff.
>
> Signed-off-by: Sergius Nyah <sergiusnyah@gmail.com>
> ---
>  t/t4018-diff-funcname.sh | 22 ++++++++++++++++++++++
>  userdiff.c               | 12 ++++++++++++
>  2 files changed, 34 insertions(+)

In t4034-diff-words.sh there is:

test_language_driver ada test_language_driver bibtex test_language_driver cpp test_language_driver csharp test_language_driver css test_language_driver dts test_language_driver fortran test_language_driver html test_language_driver java test_language_driver kotlin test_language_driver matlab test_language_driver objc test_language_driver pascal test_language_driver perl test_language_driver php test_language_driver python test_language_driver ruby test_language_driver scheme test_language_driver tex

So I would have thought that you would just add a `test_language_driver javascript` line in this file and associated material in the t/t4034/ directory.

Show 28 quoted lines

> diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh
> index e026fac1f4..d35cce18a0 100755
> --- a/t/t4018-diff-funcname.sh
> +++ b/t/t4018-diff-funcname.sh
> @@ -120,3 +120,25 @@ do
>  done
>
>  test_done
> +
> +test_expect_success 'identify builtin patterns in JavaScript' '
> +       # setup
> +       echo "function myFunction() { return true; }" > test.js &&
> +       echo "var myVar = function() { return false; }" >> test.js &&
> +       git add test.js &&
> +       git commit -m "add test.js" &&
> +
> +       # modify the file
> +       echo "function myFunction() { return false; }" > test.js &&
> +       echo "var myVar = function() { return true; }" >> test.js &&
> +
> +       # command under test
> +       git diff >output &&
> +
> +       # check results
> +       test_i18ngrep "function myFunction() { return true; }" output &&
> +       test_i18ngrep "function myFunction() { return false; }" output &&
> +       test_i18ngrep "var myVar = function() { return false; }" output &&
> +       test_i18ngrep "var myVar = function() { return true; }" output

I think we try to use just test_grep instead of test_i18ngrep these days.

> +'
> +test_done
> \ No newline at end of file

Please add a new line at the end of this file if you still change it.

Thanks!