cscorley / whatthepatch

What The Patch!? -- A Python patch parsing library
MIT License
64 stars 24 forks source link

Not parsing combined diffs correctly #23

Open ishepard opened 5 years ago

ishepard commented 5 years ago

Hi @cscorley ! I was looking for a tool to parse combined diff to use inside my tool (https://github.com/ishepard/pydriller), and ended up here. In your documentation you state you can parse the combined diff (-c or --cc option), however I am not able to obtain complete results. Let's make an example. I have this combined diff:

cc5b002a5140e2d60184de42554a8737981c846c
diff --cc tests/Microsoft.AspNet.SignalR.FunctionalTests/Client/HubProxyFacts.cs
index aaad4c4a,8bf42fc3..4979ab7d
--- a/tests/Microsoft.AspNet.SignalR.FunctionalTests/Client/HubProxyFacts.cs
+++ b/tests/Microsoft.AspNet.SignalR.FunctionalTests/Client/HubProxyFacts.cs
@@@ -36,11 -36,9 +37,11 @@@ namespace Microsoft.AspNet.SignalR.Test

                  hubConnection.Start(host.Transport).Wait();

-                 proxy.Invoke("Send", "hello").Wait();
+                 proxy.InvokeWithTimeout("Send", "hello");

 -                Assert.True(wh.WaitOne(TimeSpan.FromSeconds(5)));
 +                Assert.True(wh.WaitOne(TimeSpan.FromSeconds(10)));
 +
 +                hubConnection.Stop();
              }
          }

@@@ -65,9 -63,9 +66,9 @@@

                  hubConnection.Start(host.Transport).Wait();

-                 proxy.Invoke("Send", "hello").Wait();
+                 proxy.InvokeWithTimeout("Send", "hello");

 -                Assert.True(wh.WaitOne(TimeSpan.FromSeconds(5)));
 +                Assert.True(wh.WaitOne(TimeSpan.FromSeconds(10)));
              }
          }

diff --cc tests/Microsoft.AspNet.SignalR.FunctionalTests/Server/Hubs/HubFacts.cs
index d153740f,4bdad4db..393a1ebf
--- a/tests/Microsoft.AspNet.SignalR.FunctionalTests/Server/Hubs/HubFacts.cs
+++ b/tests/Microsoft.AspNet.SignalR.FunctionalTests/Server/Hubs/HubFacts.cs
@@@ -297,8 -291,10 +298,8 @@@ namespace Microsoft.AspNet.SignalR.Test
                      Name = "David"
                  };

-                 var person1 = hub.Invoke<SignalR.Samples.Hubs.DemoHub.DemoHub.Person>("ComplexType", person).Result;
+                 var person1 = hub.InvokeWithTimeout<SignalR.Samples.Hubs.DemoHub.DemoHub.Person>("ComplexType", person);
                  var person2 = hub.GetValue<SignalR.Samples.Hubs.DemoHub.DemoHub.Person>("person");
 -                JObject obj = ((dynamic)hub).person;
 -                var person3 = obj.ToObject<SignalR.Samples.Hubs.DemoHub.DemoHub.Person>();

                  Assert.NotNull(person1);
                  Assert.NotNull(person2);

If I run:

for diff in whatthepatch.parse_patch(text):
    pprint(diff)

I obtain:

diff(header=header(index_path=None, old_path='--cc', old_version=None, new_path='tests/Microsoft.AspNet.SignalR.FunctionalTests/Client/HubProxyFacts.cs', new_version=None), changes=None, text='diff --cc tests/Microsoft.AspNet.SignalR.FunctionalTests/Client/HubProxyFacts.cs\nindex aaad4c4a,8bf42fc3..4979ab7d\n--- a/tests/Microsoft.AspNet.SignalR.FunctionalTests/Client/HubProxyFacts.cs\n+++ b/tests/Microsoft.AspNet.SignalR.FunctionalTests/Client/HubProxyFacts.cs\n@@@ -36,11 -36,9 +37,11 @@@ namespace Microsoft.AspNet.SignalR.Test\n  \n                  hubConnection.Start(host.Transport).Wait();\n  \n-                 proxy.Invoke("Send", "hello").Wait();\n+                 proxy.InvokeWithTimeout("Send", "hello");\n  \n -                Assert.True(wh.WaitOne(TimeSpan.FromSeconds(5)));\n +                Assert.True(wh.WaitOne(TimeSpan.FromSeconds(10)));\n +\n +                hubConnection.Stop();\n              }\n          }\n  \n@@@ -65,9 -63,9 +66,9 @@@\n  \n                  hubConnection.Start(host.Transport).Wait();\n  \n-                 proxy.Invoke("Send", "hello").Wait();\n+                 proxy.InvokeWithTimeout("Send", "hello");\n  \n -                Assert.True(wh.WaitOne(TimeSpan.FromSeconds(5)));\n +                Assert.True(wh.WaitOne(TimeSpan.FromSeconds(10)));\n              }\n          }\n  \n')
diff(header=header(index_path=None, old_path='--cc', old_version=None, new_path='tests/Microsoft.AspNet.SignalR.FunctionalTests/Server/Hubs/HubFacts.cs', new_version=None), changes=None, text='diff --cc tests/Microsoft.AspNet.SignalR.FunctionalTests/Server/Hubs/HubFacts.cs\nindex d153740f,4bdad4db..393a1ebf\n--- a/tests/Microsoft.AspNet.SignalR.FunctionalTests/Server/Hubs/HubFacts.cs\n+++ b/tests/Microsoft.AspNet.SignalR.FunctionalTests/Server/Hubs/HubFacts.cs\n@@@ -297,8 -291,10 +298,8 @@@ namespace Microsoft.AspNet.SignalR.Test\n                      Name = "David"\n                  };\n  \n-                 var person1 = hub.Invoke<SignalR.Samples.Hubs.DemoHub.DemoHub.Person>("ComplexType", person).Result;\n+                 var person1 = hub.InvokeWithTimeout<SignalR.Samples.Hubs.DemoHub.DemoHub.Person>("ComplexType", person);\n                  var person2 = hub.GetValue<SignalR.Samples.Hubs.DemoHub.DemoHub.Person>("person");\n -                JObject obj = ((dynamic)hub).person;\n -                var person3 = obj.ToObject<SignalR.Samples.Hubs.DemoHub.DemoHub.Person>();\n  \n                  Assert.NotNull(person1);\n                  Assert.NotNull(person2);\n')

As you can see, old_path is "--cc", that doesn't make sense, and changes are always None.

ishepard commented 5 years ago

Actually, I just noticed that in the documentation you say "context diff", not "combined". At this point I think this is more of a feature request than an issue :)

cscorley commented 5 years ago

Hey! Big fan of the pydriller work, been following the research output for a bit. :)

Yes, this is something I think we can add from this example file. I was planning on doing a new release soon anyway. Do you have any example of what you'd expect the output structure to be? Currently each line change would just all be collapsed into one list of changes, but I'm not sure if having the source it came from would be useful or not for your use case.

ishepard commented 5 years ago

Awesome πŸ˜„ what I'd need for PyDriller is: old-path, new-path, modification type (added, deleted, etc.), and diff. So it shouldn't be too difficult to get this. We can start from these 4 things and maybe in the future we also include changes.

cscorley commented 5 years ago

Apologies for the slow work. Not a lot of free weekends this summer. I promise I started this, but got stuck :-)

ishepard commented 5 years ago

Oh no worries! I know the feeling πŸ˜„

cscorley commented 5 years ago

Didn't get to include this in the 0.0.6 release; primary problem is I think there should be a few new format fields that other formats don't need and the refactor got deep. Did want to make sure the other changes got out to the public though since it's been ... ⌚️ πŸ‘€ ... 2.5 years