AArnott / CodeGeneration.Roslyn

Assists in performing Roslyn-based code generation during a build.
Microsoft Public License
408 stars 60 forks source link

Give Generators Access to MSBuild Properties #210

Closed talenfisher closed 4 years ago

talenfisher commented 4 years ago

I have a use-case where I need to access MSBuild properties of the project I am generating code for, including TargetFrameworkVersion and OutDir, plus some custom ones. This implementation allows for that, using an opt-in approach:

  1. Add the name of the build property to the PluginRequestedProperty item list in the generator's .csproj file.
  2. Access the value via TransformationContext.BuildProperties["PropertyName"]
amis92 commented 4 years ago

Hi, thanks for the PR. This is a larger topic, so I'll come back to this PR in vNext (not 0.7).

manne commented 4 years ago

@talenfisher works like a charm!

manne commented 4 years ago

With this functionality I can access the Nullable state to generate correct nullable code

amis92 commented 4 years ago

@AArnott I'd like you to approve this before we work on it further. My opinion is that it's a nice balance between what Uno.SourceGeneration did (expose whole msbuild context) and what we want (semi-clear Compilation). We already expose some selected properties, like ProjectDirectory or DefineConstants. With this, we could just by default do <BuildProperties Include="MSBuildProjectDirectory" /> and have that covered.


@talenfisher If we get approval, I'd have some change requests.


@manne Nullable will be handled separately. I'd expect handling nullability to ship before this. This is a major extension point. We'll analyze whether it's a good idea to add.

edit: fixed itemgroup->item

AArnott commented 4 years ago

MSBuild item groups should have singular names

That's fine, but just a nit: This is an MSBuild item group:

<ItemGroup>
</ItemGroup>

These are items:

  <Something Include="a.txt" />
  <Else Include="b.txt" />

These are item types: Something, Else. And finally, this is an item list: @(Else).

Just a pet peeve of mine that people use "item group" instead of "item type". :)

manne commented 4 years ago

the functionality of this PR would fix the warnings of #190

talenfisher commented 4 years ago

@amis92 I am thinking about renaming them to RequestedProperties in the transformation context, but I'm not sure that would make sense elsewhere (the tool + engine did not request them, rather they simply passed them down). Would it be okay to leave them named as-is in the engine + tool?

talenfisher commented 4 years ago

updates 4/13 per @amis92 suggestions:

I think the doc comments make it clear enough now that these are not a 'full' dictionary of build properties, so I've left them named BuildProperties for now. I'm not against renaming them though, if that is everyone's preference