Add support for NBGV_ThisAssemblyNamespace property.

[alexrp]

Closes #555

[alexrp]

@AArnott How do you feel about the approach here?

Also, how would you suggest I test this? I'm not really familiar with NBGV's test harness.

[KalleOlaviNiemitalo]

I wonder about ThisAssemblyNamespace being null vs. an empty string. If the AssemblyVersionInfo task is called from Nerdbank.GitVersioning.targets, then what do users set in MSBuild properties to distinguish between those cases?

[KalleOlaviNiemitalo]

Here it distinguishes null from "", but if ThisAssemblyNamespace is "", then I don't think the generated F# code will compile anyway:

Nerdbank.GitVersioning/src/Nerdbank.GitVersioning.Tasks/AssemblyVersionInfo.cs

Line 124 in adb0bcd

this.generator.EmitNamespaceIfRequired(this.ThisAssemblyNamespace ?? this.RootNamespace ?? "AssemblyInfo");

Likewise C# here:

Nerdbank.GitVersioning/src/Nerdbank.GitVersioning.Tasks/AssemblyVersionInfo.cs

Lines 762 to 765 in adb0bcd

	if (ns is not null)
	{
	this.CodeBuilder.AppendLine($"namespace {ns} {{");
	}

I think those null checks should all be !IsNullOrEmpty(ns), or alternatively map "" to null before any CodeGenerator is called.

[AArnott]

@KalleOlaviNiemitalo MSBuild does not distinguish between null or an empty string as property values, nor an unset value vs. a value set to an empty string. An MSBuild task property that is set to an msbuild property will never be null, although I believe it will be null if the .targets file doesn't set the task property at all.

[KalleOlaviNiemitalo]

Well then the ?? operator in the F# case makes no sense at all.

[AArnott]

The PR looks ineffective for the other languages as well, since only F# emits a namespace. The other languages just no-op for the emit namespace instruction.

[AArnott]

@alexrp you can add the tests to this file. Please test all the languages. There is already a test for each language, so leave those alone I suppose but duplicate them with your optional property set.
Please make sure the task works properly given task inputs that are set to the empty string (indicating the msbuild property is not set).

[alexrp]

The PR looks ineffective for the other languages as well, since only F# emits a namespace. The other languages just no-op for the emit namespace instruction.

Not sure I follow here. There's code to emit namespaces for all of them. It's just that the F# case is handled specially in EmitNamespaceIfRequired().

[alexrp]

So is the conclusion that this should be changed to use string.IsNullOrEmpty() for both of these properties?

[AArnott]

Ya, that would probably be appropriate.

[AArnott]

There's code to emit namespaces for all of them.

No, I don't think so. The tests themselves prove that no namespace is emitted for C# or VB, but there is for F#. Issue #555 was originally filed because this class is always in the same global namespace for all projects.

[alexrp]

The tests themselves prove that no namespace is emitted for C# or VB

But that's expected, right? The PR makes it so that, for C#/VB, a namespace is only emitted if the project explicitly sets NBGV_ThisAssemblyNamespace to some non-empty value. For F#, a namespace is indeed always emitted as was the case prior to the PR.

I could make it always emit a namespace, but I figured that might be a bad idea for compatibility reasons.

[AArnott]

Agreed, we don't want a namespace by default. But I'm pointing out that because we never wanted a namespace before, we never implemented emitting code for the namespace, except for F# which required it. If you look at EmitNamespaceIfRequired, it is only implemented for F#, and nowhere else is the namespace produced.
If I'm wrong, can you point me to the code that emits the namespace?

[alexrp]

If I'm wrong, can you point me to the code that emits the namespace?

Here (and similarly for VB):

https://github.com/dotnet/Nerdbank.GitVersioning/pull/911/files#diff-784868d214d0f8f1823e743bdc0b7b56b4e899e08201e712ccb2bc9e35594977L756-R766

I put the code in those methods out of convenience since for C#/VB it requires opening and closing a namespace, unlike F#. (Also, I figured we shouldn't rely on file-scoped namespaces since people might be using older C# versions?) I could add a new pair of StartThisAssemblyNamespace/EndThisAssemblyNamespace methods if you think that'd be clearer.

[KalleOlaviNiemitalo]

people might be using older C# versions

Certainly are.

[AArnott]

I put the code in those methods out of convenience since for C#/VB

Ah, I missed that. I don't think we should have two separate methods for emitting namespaces.
Whether the class emitting method wraps with a namespace itself or we add before- and after- namespace methods, I don't have strong feelings. But let's just have one or the other. It's too confusing otherwise.
The One Way should probably include a parameter indicating whether the project wants the namespace emitted, so that F# can always do it and C#/VB can only do it if the caller wants it.

[KalleOlaviNiemitalo]

C# allows assembly attributes before namespace NS { … }, but not within or after it.

For F# however, BuildCode calls EmitNamespaceIfRequired before GenerateAssemblyAttributes, i.e. the attributes come after the namespace declaration. Does F# allow attributes before the namespace? If not, that gets tricky to unify. It might be best to give the language-specific classes full control on the order of calls.

[AArnott]

Bummer. Ya to give the language-specific generator more freedom, let's just set a namespace property on the class, and the generator can pick it up when it wants to.

[alexrp]

OK, here's take 2, now with tests.

[AArnott]

Looks much better. Thank you. Just one small change please.

[AArnott]

Thanks again.