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

Add support for generic attributes to StronglyTypedIdAttribute #577

Merged
merged 2 commits into from
Nov 25, 2023
Merged

Add support for generic attributes to StronglyTypedIdAttribute #577

merged 2 commits into from
Nov 25, 2023

Conversation

rfvgyhn
Copy link
Contributor

@rfvgyhn rfvgyhn commented Nov 17, 2023

This PR adds support for generic attributes to StronglyTypedIdAttribute. [StronglyTypedId<int>]

I wasn't sure what the best way to conditionally add a generic version of the attribute. This approach adds an MSBuild file that defines a preprocessor constant to check if the project supports generic attributes.

@rfvgyhn rfvgyhn requested a review from meziantou as a code owner November 17, 2023 05:13
@meziantou
Copy link
Owner

Can you rebase your branch? Also, I changed the way attributes are defined. It's now in the Meziantou.Framework.StronglyTypedId.Annotations project. You should be able to use #if NET8_0_OR_GREATER to define the attribute.

@rfvgyhn
Copy link
Contributor Author

rfvgyhn commented Nov 20, 2023

Generic attributes are a csharp 11/.net 7 feature. Do you still want to gate it behind .net8?

@meziantou
Copy link
Owner

Use the minimum supported version. If it's .net 7, use NET_7_0_OR_GREATER.

@rfvgyhn
Copy link
Contributor Author

rfvgyhn commented Nov 22, 2023

I haven't worked with multi-targeted libs before so correct me if I'm wrong on the following.

Wrapping the attribute definition in NET7_0_OR_GREATER means the reference to the Annotations project in the test project needs to get rid of the TargetFramework=netstandard2.0 property since NET7_0_OR_GREATER would always be false. That then means adding nuget references to the compilation object needs to also account for specific Microsoft.NETCore.App.Ref versions. Does that sound right?

<ProjectReference Include="..\..\src\Meziantou.Framework.StronglyTypedId.Annotations\Meziantou.Framework.StronglyTypedId.Annotations.csproj" AdditionalProperties="TargetFramework=netstandard2.0" />

private static async Task<(GeneratorDriverRunResult GeneratorResult, Compilation OutputCompilation, byte[] Assembly)> GenerateFiles(string sourceText, bool mustCompile = true)
{
var compilation = await CreateCompilation(sourceText,
[
new NuGetReference("Microsoft.NETCore.App.Ref", "6.0.12", "ref/"),
new NuGetReference("Newtonsoft.Json", "12.0.3", "lib/netstandard2.0/"),
]);

+ const string NetCoreVersion =
+ #if NET7_0
+         "7.0.14"
+ #elif NET8_0
+         "8.0.0"
+ #else
+         "6.0.12"
+ #endif
+         ;
var compilation = await CreateCompilation(sourceText,
        [
-            new NuGetReference("Microsoft.NETCore.App.Ref", "6.0.12", "ref/"),
+            new NuGetReference("Microsoft.NETCore.App.Ref", NetCoreVersion, "ref/"),

@meziantou
Copy link
Owner

Seems ok. I wonder if you can only reference the .NET 8 package in the test or if you need to use #if.

@rfvgyhn
Copy link
Contributor Author

rfvgyhn commented Nov 22, 2023

Only referencing .net8 produces CS1701 errors for most tests.

Expected result.Success to be true because Project cannot build:
error CS1701: Assuming assembly reference 'System.Runtime, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' used by 'Meziantou.Framework.StronglyTypedId.Annotations' matches identity 'System.Runtime, Version=8.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' of 'System.Runtime', you may need to supply runtime policy

I suppose that error could be suppressed via specificDiagnosticOptions. Not sure which you prefer.

I've rebased the branch. Let me know what you think.

@meziantou meziantou merged commit 6baed57 into meziantou:main Nov 25, 2023
46 of 47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants