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

Use NuGetFramework to deal with TargetFramework logic #251

Merged
merged 1 commit into from
Feb 24, 2024

Conversation

Corniel
Copy link
Contributor

@Corniel Corniel commented Feb 18, 2024

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

@rouke-broersma
Copy link

rouke-broersma commented Feb 19, 2024

@Corniel
Copy link
Contributor Author

Corniel commented Feb 20, 2024

@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
Copy link
Owner

phmonte commented Feb 20, 2024

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
Copy link

rouke-broersma commented Feb 20, 2024

@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
Copy link
Contributor Author

Corniel commented Feb 20, 2024

@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
Copy link
Owner

phmonte commented Feb 20, 2024

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 Corniel changed the title Introduction of read-only struct TargetFramework Use NuGetFramework to deal with TargetFramework logic Feb 20, 2024
@Corniel
Copy link
Contributor Author

Corniel commented Feb 20, 2024

@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).

@phmonte phmonte merged commit a4be090 into phmonte:main Feb 24, 2024
6 checks passed
@Corniel Corniel deleted the tfm branch February 24, 2024 08:16
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.

3 participants