Re: [PATCH 1/2] Subject: [GSOC][RFC PATCH 1/2] Add builtin patterns for JavaScript function detection in userdiff.
- From
Ghanshyam Thakkar <shyamthakkar001@gmail.com>
- Date
- Feb 27, 2024, 19:06 UTC
- Message-ID
- <CZG3HO25QLAG.3Q9V03SCO99HL@gmail.com>
- In-Reply-To
- <20240227160253.104011-2-sergiusnyah@gmail.com>
On Tue Feb 27, 2024 at 9:32 PM IST, Sergius Nyah wrote:
> [PATCH 1/2] Subject: [GSOC][RFC PATCH 1/2] Add builtin patterns for JavaScript function detection in userdiff.
I think these prefixes somehow got added twice. In any case, this makes it so that "Subject: [GSOC][RFC PATCH 1/2]" is part of the commit log, which shoud not be. And the subject can be better written as:
userdiff: add builtin patterns for Javascript
> This patch adds the regular expression for detecting JavaScript functions and expressions in Git diffs. The pattern accurately identifies function declerations, expressions, and definitions inside blocks.
commit message should be wrapped at 72 characters.
Show 14 quoted lines
> --- > userdiff.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/userdiff.c b/userdiff.c > index e399543823..12e31ff14d 100644 > --- a/userdiff.c > +++ b/userdiff.c > @@ -1,7 +1,7 @@ > #include "git-compat-util.h" > #include "config.h" > #include "userdiff.h" > -#include "attr.h" > +#include "attr.h"
This change adds trailing whitespace which is not desired.
Show 20 quoted lines
> #include "strbuf.h"
>
> static struct userdiff_driver *drivers;
> @@ -183,6 +183,19 @@ PATTERNS("java",
> "|[-+0-9.e]+[fFlL]?|0[xXbB]?[0-9a-fA-F]+[lL]?"
> "|[-+*/<>%&^|=!]="
> "|--|\\+\\+|<<=?|>>>?=?|&&|\\|\\|"),
> +PATTERNS("javascript",
> + /*
> + * Looks for lines that start with optional whitespace, followed
> + * by 'function'* and any characters (for function declarations),
> + * or valid JavaScript identifiers, equals sign '=', 'function' keyword
> + * and any characters (for function expressions).
> + * Also considers functions defined inside blocks with '{...}'.
> + */
> + "^[ \t]*(function[ \t]*.*|[a-zA-Z_$][0-9a-zA-Z_$]*[ \t]*=[ \t]*function[ \t]*.*|(\\{[ \t]*)?)\n",
> + /* This pattern matches JavaScript identifiers */
> + "[a-zA-Z_$][0-9a-zA-Z_$]*"
> + "|[-+0-9.eE]+|0[xX][0-9a-fA-F]+"
> + "|[-+*/<>%&^|=!:]=|--|\\+\\+|<<=?|>>=?|&&|\\|\\|"), Majority of the lines above (including comment) have trailing whitespace. Consider removing them. Also, tabs should be used for indentation instead of spaces. (cf. Documentation/CodingGuidelines)
Show 9 quoted lines
> PATTERNS("kotlin",
> "^[ \t]*(([a-z]+[ \t]+)*(fun|class|interface)[ \t]+.*)$",
> /* -- */
> @@ -192,7 +205,7 @@ PATTERNS("kotlin",
> /* integers and floats */
> "|[0-9][0-9_]*([.][0-9_]*)?([Ee][-+]?[0-9]+)?[fFlLuU]*"
> /* floating point numbers beginning with decimal point */
> - "|[.][0-9][0-9_]*([Ee][-+]?[0-9]+)?[fFlLuU]?"
> + "|[.][0-9][0-9_]*([Ee][-+]?[0-9]+ )?[fFlLuU]?"This adds extra whitespace in the middle. Changes not related to the patch topic should be avoided.
Also this was attempted before, see [1]. I think you can take some inspiration and inputs from that also. Aside from that I think the separate patch which adds tests for this can be squashed into this one.
[1]: https://lore.kernel.org/git/20220403132508.28196-1-a97410985new@gmail.com/
Thanks.