search

phmonte / Buildalyzer

A utility to perform design-time builds of .NET projects without having to think too hard about it.
MIT License
589 stars 92 forks source link

Use NuGetFramework to deal with TargetFramework logic #251

Closed Corniel closed 4 months ago

Corniel commented 4 months ago

NuGetFramework represents the target framework and its domain concerns. Thanks for the suggestion @rouke-broersma !

rouke-broersma commented 4 months ago

Note that https://www.nuget.org/packages/NuGet.Frameworks (https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.Frameworks/NuGetFramework.cs) might also work for this usecase, then you won't have to maintain the parsing yourself.

We use this ourselves here: https://github.com/stryker-mutator/stryker-net/blob/eca8545a23210f13b1ea446c52f0ad3631f4f55c/src/Stryker.Core/Stryker.Core/Initialisation/Buildalyzer/IAnalyzerResultExtensions.cs#L118

To parse the targetframework we get from buildalyzer into a more usable Type ;)

Corniel commented 4 months ago

@rouke-broersma That sounds interesting. However, Buildalizer currently does not have an dependency on NuGet, and I'm not sure if @phmonte would like to change that.

phmonte commented 4 months ago

It seems to me like a big dependency to use little resources, but if you understand that it is very important and that we have difficulty maintaining this rule, we can reconsider.

rouke-broersma commented 4 months ago

@Corniel @phmonte I understand the concern however note that it is not a dependency on Nuget, it is only the TargetFramework parsing and comparison as used by Nuget. It's a separate package that as far as I can tell does not contain anything other than functionality to work with TargetFramework, such as parsing strings to types and implementing IComparer.

See: https://github.com/NuGet/NuGet.Client/tree/dev/src/NuGet.Core/NuGet.Frameworks

The benefit would be that you would use the same resources to work with TargetFramework as Nuget does, so it's more or less guaranteed to be correct.

It was just a suggestion, I have no hard feelings one way or the other and we already parse the targetversionstring to Nuget.Frameworks ourselves anyway. If you prefer to keep your own heuristics that's fine of course.

Corniel commented 4 months ago

@rouke-broersma My personal goal is to structure this code a bit. It does awesome stuff, but is has become a challenge to maintain. Using NuGet.Frameworks might be a good step. So thanks again for the suggestion.

phmonte commented 4 months ago

Thank you very much @rouke-broersma, I had actually interpreted it incorrectly, I didn't see that there is a separate package just with NuGet.Frameworks, in this case I think it makes perfect sense.

@Corniel , I see you agree too! :)

Corniel commented 4 months ago

@phmonte Yep. Eventually, where the targetFramework is used as a string I would like to replace that with this class too, but as mentioned before: baby steps is the best approach here (and I will lead to breaking changes).