LFT-1285 - Allow users to suppress the sync generator #951
Conversation
This will allow .NET core projects to not generate sync stuff (which will often not want to support sync)
|
The first place I'd like to use this is D2L.Security.OAuth2, to remove sync from the .NET Core build. It's not so much about the implementation of the sync stuff, but the fact that the public interfaces gain sync methods and thus we're forcing users of the library to maintain sync implementations (which may not be feasible in .NET core) |
| // TODO: can we read options earlier and not spend time generating? | ||
| if( options.SuppressSyncGenerator ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
I think the easiest spot you could do is methodsToGenerate.Combine( options ).Select( GenerateSyncMethod ) and update GenerateSyncMethod to take (MethodDeclarationSyntax MethodDeclaration, Compilation Compilation, SyncGeneratorOptions options)?
There was a problem hiding this comment.
Ah whoops meant to deal with this before opening the PR.
Ya haha I was writing this in github.dev without IDE support and figured the exact signature for GenerateSyncMethod would be nasty so dodged it.
I also wanted to see if I could make it happen earlier, like before IsInterestingLookingMethod/ExtractSyntax etc. but even with the IDE it wasn't clear, oh well.
82567dd to
ec1d048
Compare
| @@ -20,7 +24,8 @@ internal sealed partial class SyncGenerator : IIncrementalGenerator { | |||
| // Do the generation per method | |||
| IncrementalValuesProvider<MethodGenerationResult> generatedMethods = | |||
| methodsToGenerate | |||
| .Select( GenerateSyncMethod ) | |||
| .Combine( options ) | |||
| .Select( (x, ct) => GenerateSyncMethod( x.Left.Item1, x.Left.Item2, x.Right, ct ) ) | |||
There was a problem hiding this comment.
x.Left.Item1, x.Left.Item2, x.Right pretty ugly lol
There was a problem hiding this comment.
Ah, I thought Combine magiced that. And you can't destructure into lambda variables yet so that's also sad
| @@ -46,4 +46,8 @@ | |||
| <PackageReference Include="System.Collections.Immutable" Version="7.0.0" PrivateAssets="All" /> | |||
| </ItemGroup> | |||
|
|
|||
| <ItemGroup> | |||
| <CompilerVisibleProperty Include="SuppressSyncGenerator" /> | |||
| </ItemGroup> | |||
There was a problem hiding this comment.
Technically not necessary but I believe putting this here will mean that users will not have to do it.
Users will configure it like this:
<PropertyGroup>
<SuppressSyncGenerator>true</SupressSyncGenerator>
</PropertyGroup>There was a problem hiding this comment.
Should we prefix that with D2LCodeStyle or something then?
There was a problem hiding this comment.
🤷♂️ I've seen microsoft not prefix as long as its unambiguous, but I have no strong feelings.
There was a problem hiding this comment.
Just noticed they do have a MyGenerator_ prefix in these docs (which I just found): https://github.com/dotnet/roslyn/blob/2946c619d91c7279d69d71bbb521ea4647e29fc1/docs/features/source-generators.cookbook.md#consume-msbuild-properties-and-metadata
This will allow .NET core projects to not generate sync stuff (which will often not want to support sync)