Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Improvement] Make VB.NET generated code consistent with C# and F# #2310

Closed
odalet opened this issue Jun 6, 2020 · 3 comments · Fixed by #2313
Closed

[Improvement] Make VB.NET generated code consistent with C# and F# #2310

odalet opened this issue Jun 6, 2020 · 3 comments · Fixed by #2313
Milestone

Comments

@odalet
Copy link
Contributor

odalet commented Jun 6, 2020

Currently, VB.NET's version of GitVersionInformation is part of the assembly's default namespace whereas F# and C# versions are part of the global namespace

Detailed Description

Similarly to C# and F# code, VB.NET's GitVersionInformation is not explicitely placed in an namespace. However, VB.NET's compiler has different namespaing rules when compared to the other two ones. Given an assembly named Foo.Bar, when one omits to specify a type's namespace, it is placed in namespace Foo.Bar. This issue is raised to submit the idea that maybe it would be better to explicitely place the GitVersionInformation in the Global namespace for consistancy purpose.

Context

It is worth mentioning here that doing so might be considered a breaking change: it all depends on whether and how existing users of GitVersion access this generated code: because the class is generated, I suppose any access to it is done through Reflection. Hence it is likely people try first to locate the GitVersionInformation class in the assembly. If they do so by explicitely providing the full name of the class (hence including its namespace), then this change will break things for these users.

On the other side, having all generated classes in the same namespace regardless of the source language would make retrieval of the GitVersionInformation class identical for all.

I'll let you decide whether the gained consistency is worth the potential break.

Possible Implementation

Would this be implemented, it'd be as simple as wrapping the existing VB.NET template within a Namespace Global. Someting like that (potentially fixing indentation and corresponding unit tests):

Namespace Global
<Global.System.Runtime.CompilerServices.CompilerGenerated()>
<Global.System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverage()>
NotInheritable Class GitVersionInformation
    Private Sub New()
    End Sub
{0}
End Class
End Namespace
@asbjornu
Copy link
Member

asbjornu commented Jun 7, 2020

I think we should do this, I'm just unsure of how much pain this is going to give consumers of GitVersion. In principle, this change warrants a major version increment (i.e. this has to be postponed to version 6), but I have a few points that may allow us to introduce this in a minor increment:

  1. The API of GitVersion that we are versioning are the external interfaces, such as the CLI for gitversion.exe and the properties supported by GitVersionTask. Consuming GitVersion as a library is not really officially supported, although it works.
  2. I assume that the Venn diagram of users of VB.NET + users of GitVersion + Does reflection on the GitVersionInformation class is so small that it's worth the risk to do this in version 5.

@odalet
Copy link
Contributor Author

odalet commented Jun 7, 2020

I had to google for 'Venn diagram' in order to be sure I understood your point :) I should point here, that the full use case also includes eg. people that use C# but consume a VB.NET assembly. Hence, the target population is maybe a bit larger, though probably not that large.
I'll soon propose a PR for this.

@github-actions
Copy link

🎉 This issue has been resolved in version 5.3.6 🎉
The release is available on:

Your GitReleaseManager bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants