Re: [GSoC][PATCH v3] Add a diff driver for JavaScript languages.

From: Glen Choo <chooglen@google.com>
Date: Apr 4, 2022, 17:32 UTC
Message-ID: <kl6ly20kycs5.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <20220403132508.28196-1-a97410985new@gmail.com>

I'm pretty happy to see follow up on this patch, I think it'll be very useful. Thanks! I think this patch has a fairly consistent direction now; though I'm undecided on whether or not I agree with the direction, I won't block this from going through.

So treat these comments as suggestions and additional JS info for the folks who actually matter :)

xing zhi jiang <a97410985new@gmail.com> writes:

Show 6 quoted lines

> In the xfunction part that matches normal functions,
> a variable declaration with an assignment of function, the function declaration
> in the class, named export for function in ECMA2015, CommonJS exports for
> named function, and also the function is object literal's property[1].
> The special match is this would match the popular framework test case
> pattern(most is same as regex for matching function in class).

With this paragraph and..

> 2. "^[\t ]*(QUnit.test\\(.*)\n"

this pattern, it looks like the direction we've chosen to take is "support popular libraries, even if this might detect the wrong thing in vanilla JS", which sounds reasonable enough to me. I think the experience would be a bit inconsistent for vanilla JS folks, but the worst thing I can imagine is that we are a bit overeager to detect headers.

As an aside, this is my first time hearing of QUnit, but I guess the JS landscape is huge and changes too often for me to keep track.

Show 8 quoted lines

>    I oberve there are many test case pattern in JavaScript. And most of popular JavaScript
>    unit test framework's test case pattern is function call can be matched by the regex, which
>    match "function in class". So it is no problem. But `QUnit framework` don't matched by that.
>    Then add this regex into JavaScript userdiff.
>    And the reason includes test case pattern has two
>    1. This is inevitable. Because we can't distinguish that with function in class.
>    2. This can help the user understand what change is happening in which test case. And if 
>       don't match the test case, it has the large possibility to match no related function.

Nit: since we're adding a pattern just for QUnit, it would be nice to
have a test for it. This would also have the nice benefit of showing why
QUnit can't make use of the other patterns.

> +	 /* popular unit testing framework test case pattern. Most of framework pattern is match by regex for "function in class" */
> +	 "^[\t ]*(QUnit.test\\(.*)\n"

This wasn't entirely clear to me on its own, but reading your --- description, I believe you mean that other unit testing frameworks are covered by another regex.

Wording suggestion, perhaps:

  /*
   * QUnit (popular unit testing framework) test case; most of the other
   * frameworks are matched by "function in class".
   */

As a technicality, there is no guarantee that the user will import the QUnit library as the variable `QUnit`, though most users will probably use `QUnit` out of convention, so I don't think this a problem [1].

[1] In case some additional context becomes useful to other reviewers:
 most JavaScript module systems do not enforce variable names when
 importing a module, so there is no guarantee that QUnit will be
 imported as the token 'QUnit', though this is often true by
 _convention_ because library authors often stick to a single name in
 their documentation.
 
 In 'old school JavaScript in the browser', libraries are often exported
 as single global variables. Those do have well-enforced names, because
 otherwise the globals would be unusable.
 
 In native ES modules (aka standardized, modern JavaScript supported by
 modern browsers and most frameworks), the library author can suggest a
 name using named exports, but with the caveats 1. the importer can
 alias the variable to a different name and 2. many authors use
 anonymous exports - either using a default export or asking users to
 import the entire library as a single, anonymous object.
 
 In CommonJS (typically used in NodeJS), one cannot name exports IIRC;
 an export is always a single, anonymous object.