Adding target framework intrinsic functions#5234
Conversation
|
@rainersigwald this will add a dependency on nuget. Do you have guidance on how to get that hooked up properly? |
Forgind
left a comment
Forgind
left a comment
There was a problem hiding this comment.
Looks good to me. Mostly curious about the tests.
rainersigwald
left a comment
rainersigwald
left a comment
There was a problem hiding this comment.
Looks good. VS and assembly-reference consistency will be a mess that might require a fairly big change in approach; let's take that one step at a time.
|
@rainersigwald I've pushed my first pass at using reflection. I've tested in VS using |
There was a problem hiding this comment.
Putting these here exposes them as [MSBuild]:: functions. That's what a lot of our stuff does. Should we consider making it more like [NuGet]::? That's a bigger change for not a ton of benefit. I think I lean toward "nah".
There was a problem hiding this comment.
Looking good to me. @nkolev92 can you take a look or nominate a NuGet person?
|
@sfoslund @rainersigwald Should this be retargeted to the 16.7 branch? |
master currently goes to VS 16.7. |
|
@rainersigwald I think I'm caught up on feedback here, unless we want to switch to |
rainersigwald
left a comment
rainersigwald
left a comment
There was a problem hiding this comment.
Can you separate out the test refactoring (adding expander and expectedMessage) from the new feature? I think this is good to go with those in individual commits.
|
@rainersigwald sounds good, just pushed those updates. 😄 |
nkolev92
left a comment
nkolev92
left a comment
There was a problem hiding this comment.
LGTM :)
I only reviewed the NuGet API usage.
| return SimpleVersion.Parse(a) <= SimpleVersion.Parse(b); | ||
| } | ||
|
|
||
| internal static string GetTargetFrameworkIdentifier(string tfm) |
There was a problem hiding this comment.
You should have a follow up issue for getting the platform (.NET 5 era intrinsics)
rainersigwald
added
the
merge-when-branch-open
ghost
added this to the
5 participants
Implementing the first few intrinsic properties from #5171