Welcome to the Git project :) I never realized that we didn't include a
built in JS diff driver, so this would be quite welcome.
I'm not entirely familiar with this part of the system or our regex
style, so I won't comment on those. I'll only comment on the patterns we
are trying to detect and whether or not we want them.
I'm not 100% clear on the style around diff drivers e.g. how do we
decide that we want a pattern or not? I'd appreciate any pointers to
docs or commits.
xing zhi jiang <a97410985new@xxxxxxxxx> writes:
> diff --git a/t/t4018/javascript-assignment-of-anonymous-function b/t/t4018/javascript-assignment-of-anonymous-function
> new file mode 100644
> index 0000000000..b6f2ccccfc
> --- /dev/null
> +++ b/t/t4018/javascript-assignment-of-anonymous-function
> @@ -0,0 +1,4 @@
> +const RIGHT = function (a, b) {
> +
> + return a + b; // ChangeMe
> +};
> diff --git a/t/t4018/javascript-assignment-of-arrow-function b/t/t4018/javascript-assignment-of-arrow-function
> new file mode 100644
> index 0000000000..24ce517b7a
> --- /dev/null
> +++ b/t/t4018/javascript-assignment-of-arrow-function
> @@ -0,0 +1,4 @@
> +const RIGHT = (a, b) => {
> +
> + return a + b; // ChangeMe
> +};
> diff --git a/t/t4018/javascript-assignment-of-arrow-function-2 b/t/t4018/javascript-assignment-of-arrow-function-2
> new file mode 100644
> index 0000000000..bbf5de369e
> --- /dev/null
> +++ b/t/t4018/javascript-assignment-of-arrow-function-2
> @@ -0,0 +1,4 @@
> +const RIGHT = (a, b)=>{
> +
> + return a + b; // ChangeMe
> +};
> diff --git a/t/t4018/javascript-assignment-of-arrow-function-3 b/t/t4018/javascript-assignment-of-arrow-function-3
> new file mode 100644
> index 0000000000..4a07aa3259
> --- /dev/null
> +++ b/t/t4018/javascript-assignment-of-arrow-function-3
> @@ -0,0 +1,4 @@
> +const RIGHT=test=>{
> +
> + return test + 1; // ChangeMe
> +};
These are all variable assignments of anonymous functions, so we won't
'technically' be showing the function name in the diff, but as a
practical matter, they are _probably_ referred to by this name
consistently. So including them makes sense.
> diff --git a/t/t4018/javascript-assignment-of-named-function b/t/t4018/javascript-assignment-of-named-function
> new file mode 100644
> index 0000000000..bfc486ebef
> --- /dev/null
> +++ b/t/t4018/javascript-assignment-of-named-function
> @@ -0,0 +1,4 @@
> +const RIGHT = function test (a, b) {
> +
> + return a + b; // ChangeMe
> +};
> diff --git a/t/t4018/javascript-async-function b/t/t4018/javascript-async-function
> new file mode 100644
> index 0000000000..993e6926bf
> --- /dev/null
> +++ b/t/t4018/javascript-async-function
> @@ -0,0 +1,4 @@
> +async function RIGHT(a, b) {
> +
> + return a + b; // ChangeMe
> +}
> diff --git a/t/t4018/javascript-export-async-function b/t/t4018/javascript-export-async-function
> new file mode 100644
> index 0000000000..fecbd669d7
> --- /dev/null
> +++ b/t/t4018/javascript-export-async-function
> @@ -0,0 +1,4 @@
> +export async function RIGHT(a, b) {
> +
> + return a + b; // ChangeMe
> +}
> diff --git a/t/t4018/javascript-export-function b/t/t4018/javascript-export-function
> new file mode 100644
> index 0000000000..b5acbb2b08
> --- /dev/null
> +++ b/t/t4018/javascript-export-function
> @@ -0,0 +1,4 @@
> +export function RIGHT(a, b) {
> +
> + return a + b; // ChangeMe
> +}
These look good; the 'export' statements are part of the ES modules
feature. I'm not sure if it makes sense to explicitly test these cases
unless we have reason to believe that the 'export' keyword will affect
the matching.
> diff --git a/t/t4018/javascript-exports-anomyous-function b/t/t4018/javascript-exports-anomyous-function
> new file mode 100644
> index 0000000000..6786cbda8d
> --- /dev/null
> +++ b/t/t4018/javascript-exports-anomyous-function
> @@ -0,0 +1,4 @@
> +exports.setFlagged = RIGHT => {
> +
> + return ChangeMe;
> +};
> diff --git a/t/t4018/javascript-exports-anomyous-function-2 b/t/t4018/javascript-exports-anomyous-function-2
> new file mode 100644
> index 0000000000..883569f40d
> --- /dev/null
> +++ b/t/t4018/javascript-exports-anomyous-function-2
> @@ -0,0 +1,4 @@
> +exports.RIGHT = (a, b, runtime) => {
> +
> + return ChangeMe;
> +};
> diff --git a/t/t4018/javascript-exports-function b/t/t4018/javascript-exports-function
> new file mode 100644
> index 0000000000..63b79f5991
> --- /dev/null
> +++ b/t/t4018/javascript-exports-function
> @@ -0,0 +1,4 @@
> +exports.RIGHT = function(document) {
> +
> + return ChangeMe;
> +}
I don't think we should include 'exports.foo = bar'. To my knowledge,
this is _not_ a standard ES feature, but rather the CommonJS module
system popularized by Node.js [1] and other frameworks. To confirm this,
I searched the ES spec and did not find any reference to exports.* [2].
Even if we wanted to support nonstandard 'language features' (and this
label is tenuous at best, CommonJS is not trying to replace the ES
modules standard AFAIK), Node.js is also starting to support ES modules,
so I don't think this is a good long term direction for Git.
[1] https://nodejs.org/api/modules.html
[2] https://262.ecma-international.org/12.0/#sec-exports
> diff --git a/t/t4018/javascript-function b/t/t4018/javascript-function
> new file mode 100644
> index 0000000000..0cc0bf54e7
> --- /dev/null
> +++ b/t/t4018/javascript-function
> @@ -0,0 +1,4 @@
> +function RIGHT(a, b) {
> +
> + return a + b; // ChangeMe
> +}
> diff --git a/t/t4018/javascript-function-2 b/t/t4018/javascript-function-2
> new file mode 100644
> index 0000000000..06cfb779f0
> --- /dev/null
> +++ b/t/t4018/javascript-function-2
> @@ -0,0 +1,10 @@
> +function test(a, b) {
> + return {
> + RIGHT: function () {
> + currentUpdateRemovedChunks.forEach(function (chunkId) {
> + delete $installedChunks$[chunkId];
> + });
> + currentUpdateRemovedChunks = ChangeMe;
> + }
> + }
> +}
There is also the ES2015 'method shorthand' syntax [3], e.g. `bar` in:
const foo = {
bar() {
console.log('hi');
}
}
[3] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Method_definitions
> diff --git a/t/t4018/javascript-function-belong-to-IIFE b/t/t4018/javascript-function-belong-to-IIFE
> new file mode 100644
> index 0000000000..6e5fe858c0
> --- /dev/null
> +++ b/t/t4018/javascript-function-belong-to-IIFE
> @@ -0,0 +1,6 @@
> +(function () {
> + this.$RIGHT = function (needle, modifier) {
> + let a = 5;
> + return ChangeMe;
> + };
> +}).call(aaaa.prototype);
Does the IIFE matter in this case? This line:
this.$RIGHT = function (needle, modifier) {
looks extremely similar to the previous test of `foo = function bar()`.
Or perhaps this is meant to demonstrate the edge case of "matching in a
complicated construct"? If so, perhaps we should test other edge cases
like:
function WRONG() {
let RIGHT = function (needle, modifier) {
let a = 5;
return ChangeMe;
};
}
> diff --git a/t/t4018/javascript-function-in-class b/t/t4018/javascript-function-in-class
> new file mode 100644
> index 0000000000..0cc0a26612
> --- /dev/null
> +++ b/t/t4018/javascript-function-in-class
> @@ -0,0 +1,6 @@
> +class Test {
> + RIGHT() {
> + let a = 4;
> + let b = ChangeMe;
> + }
> +}
> diff --git a/t/t4018/javascript-function-in-class-2 b/t/t4018/javascript-function-in-class-2
> new file mode 100644
> index 0000000000..725495fe55
> --- /dev/null
> +++ b/t/t4018/javascript-function-in-class-2
> @@ -0,0 +1,11 @@
> +class Test {
> + RIGHT(
> + aaaaaaaaaa,
> + bbbbbbbbbb,
> + cccccccccc,
> + dddddddddd
> + ) {
> + let a = 4;
> + let b = ChangeMe;
> + }
> +}
> diff --git a/t/t4018/javascript-function-in-class-3 b/t/t4018/javascript-function-in-class-3
> new file mode 100644
> index 0000000000..e9b20728b2
> --- /dev/null
> +++ b/t/t4018/javascript-function-in-class-3
> @@ -0,0 +1,10 @@
> +class Test {
> + RIGHT(aaaaaaaaaa,
> + bbbbbbbbbb,
> + cccccccccc,
> + dddddddddd
> + ) {
> + let a = 4;
> + let b = ChangeMe;
> + }
> +}
> diff --git a/t/t4018/javascript-function-in-object-literal b/t/t4018/javascript-function-in-object-literal
> new file mode 100644
> index 0000000000..021cc706dd
> --- /dev/null
> +++ b/t/t4018/javascript-function-in-object-literal
> @@ -0,0 +1,7 @@
> +const obj = {
> + RIGHT: function (elems, callback, arg) {
> + var length, value;
> + // ...
> + return ChangeMe
> + }
> +}
> diff --git a/t/t4018/javascript-generator-function b/t/t4018/javascript-generator-function
> new file mode 100644
> index 0000000000..dc7793939f
> --- /dev/null
> +++ b/t/t4018/javascript-generator-function
> @@ -0,0 +1,4 @@
> +function* RIGHT(a, b) {
> +
> + return a + b; // ChangeMe
> +}
> diff --git a/t/t4018/javascript-generator-function-2 b/t/t4018/javascript-generator-function-2
> new file mode 100644
> index 0000000000..950676a612
> --- /dev/null
> +++ b/t/t4018/javascript-generator-function-2
> @@ -0,0 +1,4 @@
> +function *RIGHT(a, b) {
> +
> + return a + b; // ChangeMe
> +}
> diff --git a/t/t4018/javascript-getter-function-in-class b/t/t4018/javascript-getter-function-in-class
> new file mode 100644
> index 0000000000..9a5aee39f7
> --- /dev/null
> +++ b/t/t4018/javascript-getter-function-in-class
> @@ -0,0 +1,6 @@
> +class Test {
> + get RIGHT() {
> + let a = 4;
> + let b = ChangeMe;
> + }
> +}
> diff --git a/t/t4018/javascript-setter-function-in-class b/t/t4018/javascript-setter-function-in-class
> new file mode 100644
> index 0000000000..dc5f288665
> --- /dev/null
> +++ b/t/t4018/javascript-setter-function-in-class
> @@ -0,0 +1,6 @@
> +class Test {
> + set RIGHT() {
> + let a = 4;
> + let b = ChangeMe;
> + }
> +}
> diff --git a/t/t4018/javascript-skip-function-call-statement b/t/t4018/javascript-skip-function-call-statement
> new file mode 100644
> index 0000000000..321993c27e
> --- /dev/null
> +++ b/t/t4018/javascript-skip-function-call-statement
> @@ -0,0 +1,7 @@
> +class Test {
> + static RIGHT() {
> + haha();
> + haha2()
> + let b = ChangeMe;
> + }
> +}
> diff --git a/t/t4018/javascript-skip-keywords b/t/t4018/javascript-skip-keywords
> new file mode 100644
> index 0000000000..5584970b58
> --- /dev/null
> +++ b/t/t4018/javascript-skip-keywords
> @@ -0,0 +1,34 @@
> +function RIGHT(a, b) {
> + import("./async1")
> + if (a > 1) {
> + // ...
> + }
> + do {
> + // ...
> + } while (i < 5);
> + for (const element of array1) {
> + console.log(element)
> + }
> + with(o) {
> + console.log(x)
> + }
> + switch (expr) {
> + case 'a':
> + // ...
> + break;
> + case 'b':
> + // ...
> + break;
> + default:
> + // ...
> + }
> + try {
> + // ...
> + return (a + c)
> + }
> + catch (error) {
> + // ...
> + }
> +
> + return a + b; // ChangeMe
> +}
> diff --git a/t/t4018/javascript-static-function-in-class b/t/t4018/javascript-static-function-in-class
> new file mode 100644
> index 0000000000..fbf0b7ca3d
> --- /dev/null
> +++ b/t/t4018/javascript-static-function-in-class
> @@ -0,0 +1,6 @@
> +class Test {
> + static RIGHT() {
> + let a = 4;
> + let b = ChangeMe;
> + }
> +}
The rest of the test cases look good.
