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 0000000..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 0000000..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 0000000..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 0000000..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 0000000..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 0000000..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 0000000..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 0000000..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 undecorated (no "unsafe" etc.)