Re: [PATCH 2/2] add csharp userdiff tests
Am 27.04.2014 18:46, schrieb Marius Ungureanu: > Is it okay though if I add a few tests to show what is broken? > > I think this can’t be solved at a regex level. It's OK to add tests that show breakages even if there is no immediate solution. >> You can mark a userdiff test case that demonstrates a breakage by >> including the work "broken" somewhere in the file. See >> http://www.repo.or.cz/w/alt-git.git/commitdiff/9cc444f0570b196f1c51664ce2de1d8e1dee6046 You add tests including broken cases first, and then in the follow-up patch that fixes the broken ones, you also mark the tests as fixed, like I did in the follow-up patch of the above example: http://www.repo.or.cz/w/alt-git.git/commitdiff/8a2e8da367f7175465118510b474ad365161d6b1 -- Hannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] add csharp userdiff tests
> You can mark a userdiff test case that demonstrates a breakage by > including the work "broken" somewhere in the file. See > http://www.repo.or.cz/w/alt-git.git/commitdiff/9cc444f0570b196f1c51664ce2de1d8e1dee6046 > -- Hannes Would you like me to add tests first, then fix them? Or just like this?-- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] add csharp userdiff tests
On 27 Apr 2014, at 19:19, Johannes Sixt wrote: > Am 27.04.2014 15:47, schrieb Marius Ungureanu: >> >> --- > > Thanks. Please signed off your patch. > Ah, yes, forgot to do that. > When you re-send, please place [PATCH v3 n/m] in the subject (and drop > the "Re:") and note what you changed compared to the previous (or all > earlier) rounds. The place for this note is here, after the "---" marker. > > Have a look at Documentation/SubmittingPatches. > Will do so. >> t/t4018/csharp-constructor | 4 >> t/t4018/csharp-destructor | 4 >> t/t4018/csharp-function| 4 >> t/t4018/csharp-member | 4 >> t/t4018/csharp-namespace | 4 >> t/t4018/csharp-operator| 4 >> t/t4018/csharp-property| 4 >> t/t4018/csharp-skip-call | 5 + >> 8 files changed, 33 insertions(+) >> create mode 100644 t/t4018/csharp-constructor >> create mode 100644 t/t4018/csharp-destructor >> create mode 100644 t/t4018/csharp-function >> create mode 100644 t/t4018/csharp-member >> create mode 100644 t/t4018/csharp-namespace >> create mode 100644 t/t4018/csharp-operator >> create mode 100644 t/t4018/csharp-property >> create mode 100644 t/t4018/csharp-skip-call >> > > Unfortunately, I think you have reduced the test cases too far. One of > the main properties of C# code is that usually member and property > definitions are indented and there is a class definition around them: > > class Foo { > Foo(X) {} > virtual void DoStuff() {} > public int X; > }; > > In your examples, you omitted the surrounding class definition and did > not indent the member definitions. By doing so, the test cases do not > demonstrate that the csharp userdiff patterns are significantly > different from the default userdiff pattern: in the examples you > present, the default pattern would have picked the same hunk headers as > the csharp patterns! > Ah, I think I over judged minimal sample here. I’ll do so. Is it okay though if I add a few tests to show what is broken? I think this can’t be solved at a regex level. > For a reviewer who is not (or only marginally) familiar with C# (like > myself), it would have been very instructive to present patches with > test cases that demonstrate weaknesses of the old patterns before > patches that fix them. For example, you say that you fix the constructor > pattern. But I am unable to judge what is wrong and how you fix it. The > commit message is the right place to add text that helps reviewers. > Well, the previous pattern didn’t match constructors as they should be at a logical level. That means, it considered the constructor name as being the return type. It’s just a logical change that helped with writing operator function parsing. > You can mark a userdiff test case that demonstrates a breakage by > including the work "broken" somewhere in the file. See > http://www.repo.or.cz/w/alt-git.git/commitdiff/9cc444f0570b196f1c51664ce2de1d8e1dee6046 > >> diff --git a/t/t4018/csharp-constructor b/t/t4018/csharp-constructor >> new file mode 100644 >> index 000..a39cffb >> --- /dev/null >> +++ b/t/t4018/csharp-constructor >> @@ -0,0 +1,4 @@ >> +MyClass(RIGHT) >> +{ >> +ChangeMe; >> +} >> diff --git a/t/t4018/csharp-destructor b/t/t4018/csharp-destructor >> new file mode 100644 >> index 000..55106d9 >> --- /dev/null >> +++ b/t/t4018/csharp-destructor >> @@ -0,0 +1,4 @@ >> +~MyClass(RIGHT) >> +{ >> +ChangeMe; >> +} >> diff --git a/t/t4018/csharp-function b/t/t4018/csharp-function >> new file mode 100644 >> index 000..a5d59a3 >> --- /dev/null >> +++ b/t/t4018/csharp-function >> @@ -0,0 +1,4 @@ >> +virtual DoStuff(RIGHT) >> +{ >> +ChangeMe; >> +} >> diff --git a/t/t4018/csharp-member b/t/t4018/csharp-member >> new file mode 100644 >> index 000..4939d53 >> --- /dev/null >> +++ b/t/t4018/csharp-member >> @@ -0,0 +1,4 @@ >> +unsafe class RIGHT >> +{ >> +int ChangeMe; >> +} >> diff --git a/t/t4018/csharp-namespace b/t/t4018/csharp-namespace >> new file mode 100644 >> index 000..6749980 >> --- /dev/null >> +++ b/t/t4018/csharp-namespace >> @@ -0,0 +1,4 @@ >> +namespace RIGHT >> +{ >> +ChangeMe; >> +} >> diff --git a/t/t4018/csharp-operator b/t/t4018/csharp-operator >> new file mode 100644 >> index 000..5b00263 >> --- /dev/null >> +++ b/t/t4018/csharp-operator > > "csharp-user-defined-operator" would more precisely describe the case. I > wouldn't mind seening other file names being a bit more elaborate, but I > find this one particularly ambiguous. > Okay, I’ll name them better. Also improve them. >> @@ -0,0 +1,4 @@ >> +A operator+(RIGHT) >> +{ >> +ChangeMe; >> +} >> diff --git a/t/t4018/csharp-property b/t/t4018/csharp-property >> new file mode 100644 >> index 000..82d67fc >> --- /dev/null >> +++ b/t/t4018/csharp-property >> @@ -0,0 +1,4 @@ >> +public virtual int RIGHT >> +{ >> +get { ChangeMe; } >> +} >> diff --git a/t/t4018/csharp-skip-call b/t/t4018/csharp-skip-call >> new file mode 100644 >> index
Re: [PATCH 2/2] add csharp userdiff tests
Am 27.04.2014 15:47, schrieb Marius Ungureanu: > > --- Thanks. Please signed off your patch. When you re-send, please place [PATCH v3 n/m] in the subject (and drop the "Re:") and note what you changed compared to the previous (or all earlier) rounds. The place for this note is here, after the "---" marker. Have a look at Documentation/SubmittingPatches. > t/t4018/csharp-constructor | 4 > t/t4018/csharp-destructor | 4 > t/t4018/csharp-function| 4 > t/t4018/csharp-member | 4 > t/t4018/csharp-namespace | 4 > t/t4018/csharp-operator| 4 > t/t4018/csharp-property| 4 > t/t4018/csharp-skip-call | 5 + > 8 files changed, 33 insertions(+) > create mode 100644 t/t4018/csharp-constructor > create mode 100644 t/t4018/csharp-destructor > create mode 100644 t/t4018/csharp-function > create mode 100644 t/t4018/csharp-member > create mode 100644 t/t4018/csharp-namespace > create mode 100644 t/t4018/csharp-operator > create mode 100644 t/t4018/csharp-property > create mode 100644 t/t4018/csharp-skip-call > Unfortunately, I think you have reduced the test cases too far. One of the main properties of C# code is that usually member and property definitions are indented and there is a class definition around them: class Foo { Foo(X) {} virtual void DoStuff() {} public int X; }; In your examples, you omitted the surrounding class definition and did not indent the member definitions. By doing so, the test cases do not demonstrate that the csharp userdiff patterns are significantly different from the default userdiff pattern: in the examples you present, the default pattern would have picked the same hunk headers as the csharp patterns! For a reviewer who is not (or only marginally) familiar with C# (like myself), it would have been very instructive to present patches with test cases that demonstrate weaknesses of the old patterns before patches that fix them. For example, you say that you fix the constructor pattern. But I am unable to judge what is wrong and how you fix it. The commit message is the right place to add text that helps reviewers. You can mark a userdiff test case that demonstrates a breakage by including the work "broken" somewhere in the file. See http://www.repo.or.cz/w/alt-git.git/commitdiff/9cc444f0570b196f1c51664ce2de1d8e1dee6046 > diff --git a/t/t4018/csharp-constructor b/t/t4018/csharp-constructor > new file mode 100644 > index 000..a39cffb > --- /dev/null > +++ b/t/t4018/csharp-constructor > @@ -0,0 +1,4 @@ > +MyClass(RIGHT) > +{ > + ChangeMe; > +} > diff --git a/t/t4018/csharp-destructor b/t/t4018/csharp-destructor > new file mode 100644 > index 000..55106d9 > --- /dev/null > +++ b/t/t4018/csharp-destructor > @@ -0,0 +1,4 @@ > +~MyClass(RIGHT) > +{ > + ChangeMe; > +} > diff --git a/t/t4018/csharp-function b/t/t4018/csharp-function > new file mode 100644 > index 000..a5d59a3 > --- /dev/null > +++ b/t/t4018/csharp-function > @@ -0,0 +1,4 @@ > +virtual DoStuff(RIGHT) > +{ > + ChangeMe; > +} > diff --git a/t/t4018/csharp-member b/t/t4018/csharp-member > new file mode 100644 > index 000..4939d53 > --- /dev/null > +++ b/t/t4018/csharp-member > @@ -0,0 +1,4 @@ > +unsafe class RIGHT > +{ > + int ChangeMe; > +} > diff --git a/t/t4018/csharp-namespace b/t/t4018/csharp-namespace > new file mode 100644 > index 000..6749980 > --- /dev/null > +++ b/t/t4018/csharp-namespace > @@ -0,0 +1,4 @@ > +namespace RIGHT > +{ > + ChangeMe; > +} > diff --git a/t/t4018/csharp-operator b/t/t4018/csharp-operator > new file mode 100644 > index 000..5b00263 > --- /dev/null > +++ b/t/t4018/csharp-operator "csharp-user-defined-operator" would more precisely describe the case. I wouldn't mind seening other file names being a bit more elaborate, but I find this one particularly ambiguous. > @@ -0,0 +1,4 @@ > +A operator+(RIGHT) > +{ > + ChangeMe; > +} > diff --git a/t/t4018/csharp-property b/t/t4018/csharp-property > new file mode 100644 > index 000..82d67fc > --- /dev/null > +++ b/t/t4018/csharp-property > @@ -0,0 +1,4 @@ > +public virtual int RIGHT > +{ > + get { ChangeMe; } > +} > diff --git a/t/t4018/csharp-skip-call b/t/t4018/csharp-skip-call > new file mode 100644 > index 000..e89d083 > --- /dev/null > +++ b/t/t4018/csharp-skip-call > @@ -0,0 +1,5 @@ > +virtual void RIGHT() > +{ > + call(); > + ChangeMe; > +} In this last test case, you want to demonstrate that the line "call()" is not picked as hunk header. As written, the line would never be picked as hunk header, even if it would match some pattern, because it is too close to "ChangeMe". You must have at least one more line between "call()" and "ChangeMe". BTW, what is the expected hunk header in a diff like the following where "class Foo" is at line 1 in the file (just above the hunk)? @@ -2,3 +2,3 @@ { - // old comment + // new comment public whatever() That is, when the class definition is u
Re: [PATCH 2/2] add csharp userdiff tests
Ugh, there’s an empty line at the beginning of the 2nd patch that shouldn’t be there. Thanks, Marius-- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] add csharp userdiff tests
--- t/t4018/csharp-constructor | 4 t/t4018/csharp-destructor | 4 t/t4018/csharp-function| 4 t/t4018/csharp-member | 4 t/t4018/csharp-namespace | 4 t/t4018/csharp-operator| 4 t/t4018/csharp-property| 4 t/t4018/csharp-skip-call | 5 + 8 files changed, 33 insertions(+) create mode 100644 t/t4018/csharp-constructor create mode 100644 t/t4018/csharp-destructor create mode 100644 t/t4018/csharp-function create mode 100644 t/t4018/csharp-member create mode 100644 t/t4018/csharp-namespace create mode 100644 t/t4018/csharp-operator create mode 100644 t/t4018/csharp-property create mode 100644 t/t4018/csharp-skip-call diff --git a/t/t4018/csharp-constructor b/t/t4018/csharp-constructor new file mode 100644 index 000..a39cffb --- /dev/null +++ b/t/t4018/csharp-constructor @@ -0,0 +1,4 @@ +MyClass(RIGHT) +{ + ChangeMe; +} diff --git a/t/t4018/csharp-destructor b/t/t4018/csharp-destructor new file mode 100644 index 000..55106d9 --- /dev/null +++ b/t/t4018/csharp-destructor @@ -0,0 +1,4 @@ +~MyClass(RIGHT) +{ + ChangeMe; +} diff --git a/t/t4018/csharp-function b/t/t4018/csharp-function new file mode 100644 index 000..a5d59a3 --- /dev/null +++ b/t/t4018/csharp-function @@ -0,0 +1,4 @@ +virtual DoStuff(RIGHT) +{ + ChangeMe; +} diff --git a/t/t4018/csharp-member b/t/t4018/csharp-member new file mode 100644 index 000..4939d53 --- /dev/null +++ b/t/t4018/csharp-member @@ -0,0 +1,4 @@ +unsafe class RIGHT +{ + int ChangeMe; +} diff --git a/t/t4018/csharp-namespace b/t/t4018/csharp-namespace new file mode 100644 index 000..6749980 --- /dev/null +++ b/t/t4018/csharp-namespace @@ -0,0 +1,4 @@ +namespace RIGHT +{ + ChangeMe; +} diff --git a/t/t4018/csharp-operator b/t/t4018/csharp-operator new file mode 100644 index 000..5b00263 --- /dev/null +++ b/t/t4018/csharp-operator @@ -0,0 +1,4 @@ +A operator+(RIGHT) +{ + ChangeMe; +} diff --git a/t/t4018/csharp-property b/t/t4018/csharp-property new file mode 100644 index 000..82d67fc --- /dev/null +++ b/t/t4018/csharp-property @@ -0,0 +1,4 @@ +public virtual int RIGHT +{ + get { ChangeMe; } +} diff --git a/t/t4018/csharp-skip-call b/t/t4018/csharp-skip-call new file mode 100644 index 000..e89d083 --- /dev/null +++ b/t/t4018/csharp-skip-call @@ -0,0 +1,5 @@ +virtual void RIGHT() +{ + call(); + ChangeMe; +} -- 1.8.4 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html