VB -> C#: private partial methods do not compile

[jrmoreno1]

• Added unit test to SpecialConversionTests (not MethodStatementTests as there is no statements) -ISymbolExtensions.cs
  • CanHaveMethodBody & IsPartialMethodDefinition
  • Added check for IsPartialDefinition

#1097

Problem

Partial method declarations are including a body, which makes them partial method IMPLEMENTATIONS without declarations and so the resulting conversion does not compile

Solution

• Changed the code as little as possible, I think it fits in with the existing code, could be broken up into multiple (shorter) lines, and negative pattern matching could probably be used, but didn't want to change too much.
• Change to IsPartialMethodDefinition is unnecessary for this, not sure it should be included or not.
• There is a test for problem, but there's not a test for the other end of it (i.e. that it correctly converts partial implementations, but I didn't have a problem with those).
• At least one test covering the code changed

[jrmoreno1]

@GrahamTheCoder : no test results have changed, but I did add a unit test for it. Not sure it's where you'll want it.

[GrahamTheCoder]

Thanks this seems right to me - I see that IsPartialMethodDefinition can be true even when neither of the implementationpart and definitionpart are set - that was the thing that made it look odd to me I think.

I'll merge it in its current state and make a few tweaks on top

Change to IsPartialMethodDefinition is unnecessary for this, not sure it should be included or not.

As a general principle: if in doubt, and you don't have a test case that shows the issue, don't change the production behaviour.
But you should ask "why did I want to change it?". It's because you spotted it's duplicate code. So in that case the answer is to deduplicate it. CanHaveMethodBody should call IsPartialMethodDefinition, and then the change is (at least partially) covered by your test. I've deduped the logic in 4d19fd8

Test placement

I put the unit test in SpecialConversionTest instead of MethodStatementTests (as the method doesn't have any statements)

I'd suggest MemberTests is actually the best place so I've moved it there in 81ff635
Apologies that "Special" isn't a great name since it can cover many things, but therefore we should always use the more specific test files when possible, espcially since this is just straightforward like-for-like conversion of a class member.
A good way to find a place is to just put it near the most similar tests, though with a quick regex look for partial.*(sub|function) only showed up this one:

CodeConverter/Tests/CSharp/MemberTests/MemberTests.cs

Lines 895 to 921 in add696c

	public async Task PartialClassAsync()
	{
	await TestConversionVisualBasicToCSharpAsync(
	@"Public Partial Class TestClass
	Private Sub DoNothing()
	Console.WriteLine(""Hello"")
	End Sub
	End Class
	Class TestClass ' VB doesn't require partial here (when just a single class omits it)
	Partial Private Sub DoNothing()
	End Sub
	End Class",
	@"using System;
	public partial class TestClass
	{
	partial void DoNothing()
	{
	Console.WriteLine(""Hello"");
	}
	}
	public partial class TestClass // VB doesn't require partial here (when just a single class omits it)
	{
	partial void DoNothing();
	}");

Style

I didn't want to change too much

I'll do an example commit of the style I'd aim for in any new code just to demonstrate the direction, but definitely the best plan to stick with the surrounding code by default, minimum change prior to review is generally simplest.

Declarative is generally preferable, so any use of pattern matching is good, but best to keep style changes in separate post-review commits or PRs.

tl; dr

Good to merge 🚀 (so long as I didn't break anything whilst showing the style we're aiming for)