Make it an Incremental Source Generator #6
Conversation
| out var stripMetadata | ||
| ); | ||
|
|
||
| // Afaik these should always be set. |
There was a problem hiding this comment.
Not true. <GenerateAssemblyInfo>false</GenerateAssemblyInfo>
There was a problem hiding this comment.
Even if I set that to false, these values are still set to defaults which I think is the csproj file name and then 1.0.0 for the version. These values are exposed using the <CompilerVisibleProperty Include="NameOfProperty" /> things in the BepInEx.AutoPlugin.props file, and those values are written to a <NameOfProject>.GeneratedMSBuildEditorConfig.editorconfig file where all the available properties are for us to use.
If the user however does not reference the build files from this package, then these values would be null.
There was a problem hiding this comment.
Okay reading again what I said, what I meant was that even with <GenerateAssemblyInfo>false</GenerateAssemblyInfo> set the info is still all there, but if I don't set the relevant properties in the csproj, then it will be the defaults which the SDK sets for those properties.
|
I implemented an analyzer for the missing partial modifiers now. Not sure about its quality though since I have barely any experience with analyzers. |
|
I would say that now this is in a pretty good shape as I think all issues are resolved (at least to me) and the readme is no longer outdated. |
| var (options, isBepInEx5) = info; | ||
| var globalOptions = options.GlobalOptions; | ||
| globalOptions.TryGetValue("build_property.AssemblyName", out var assemblyName); | ||
| globalOptions.TryGetValue("build_property.Product", out var productName); |
There was a problem hiding this comment.
What's the rationale for switching from Title to Product?
Are these properties even documented somewhere? The only difference between these two I see is that Title shows up in windows's file properties.
There was a problem hiding this comment.
Oh, yeah I didn't even realize this used Title and not Product like PluginInfoProps, so there is really no reason for the change. But now that I looked at it, this stackoverflow answer https://stackoverflow.com/a/30219774 makes it sound like Actually never mind I saw an image of what it looks like, and I feel like Product would be the more appropriate property for the project name.Title would be very appropriate here.
Looking at how they behave, Product set is to a default value (seems like assembly name), but title is not set to anything.
Since PluginInfoProps uses Product and AutoPlugin has used Title, we could take Title first and check if it's defined, and if yes, use it, if not, use Product as the project name.
…ginStripBuildMetadata option it turns out trying to take the info from MSBuild properties isn't reliable because why would it be.
|
Okay, so now the <PropertyGroup>
<BepInAutoPluginStripBuildMetadata>true</BepInAutoPluginStripBuildMetadata>
</PropertyGroup>This uses InformationalVersionAttribute for the version even though I said it's wrong to use because of https://learn.microsoft.com/en-us/dotnet/standard/assembly/versioning#assembly-informational-version but honestly I don't care |
This is probably slightly faster than using ForAttributeWithMetadataName based on my terrible benchmarks, and where FAWMN shines is attributes that don't exist, but these attributes should always exist.
|
I switched off from using FAWMN since it seems like it's probably slightly faster this way. |
it's probably better to just ignore everything since the user can't do anything about it if they have some analyzer that that doesn't like what's happening there. One issue is when publicizing an assembly which also uses AutoPlugin.
3 participants
As of right now, BepInEx.AutoPlugin is a non-incremental source generator. By making it incremental, it will feel much more responsive and hopefully will lead to less confusion, as it will regenerate the classes in real time when needed.
I might have modified more than I should about the project, but it hasn't been touched in a while so I did what I wanted to do, especially considering that this is basically a complete rewrite anyways.
Note: this extends #5
Also worth noting that I implemented this too which I mentioned about previously (Edit: now removed since this isn't actually needed when taking properties from msbuild (Edit: this removal has been reverted due to changing back to taking info from attributes))