add in notes on plan for net5 platform feature #9382
Conversation
zkat
requested review from
dominoFire,
zivkan,
rrelyea,
heng-liu,
donnie-msft and
kartheekp-ms
zkat
force-pushed
the
dev-zkat-net5-platforms
branch
from
361174b to
6de781a
Compare
zkat
force-pushed
the
dev-zkat-net5-platforms
branch
from
6de781a to
363e4af
Compare
| /// </summary> | ||
| public bool HasPlatformVersion | ||
| { | ||
| get { return IsNet5Era && !(PlatformVersion.Major == 0 && PlatformVersion.Minor == 0); } |
There was a problem hiding this comment.
I'm not sure the comment matches the logic, and there may be a better way of identifying "empty" versions. My understanding from the Version Class is that -1 is the unassigned value, and 0.0 would be a valid version:
The value of Version properties that have not been explicitly assigned a value is undefined (-1).
There was a problem hiding this comment.
Also, what's wrong will null? This is a valid use for null values. It seems wasteful to force the property to be not-null, and then invent our own convention for how to represent "not specified"
There was a problem hiding this comment.
The "no null" thing is something I pushed for.
The reason is consistency.
The platform version is expected to be declared in a similar way as the target framework version, see: https://nugettoolsdev.azurewebsites.net/5.5.0-preview.2.6382/parse-framework?framework=net.
The serialization should be predictable too.
You'd get a v0.0 and that's a good enough experience.
If you have a TPI, you need a TPV.
TPI needs a valid value to create a NuGetFramework, having a not declared TPV is weird. We need a default that makes more sense.
0.0 is what TFV does, so we should do the same.
There was a problem hiding this comment.
yeah, this was a long conversation. If you still want to chat about it, we can jump on a call. Let me know, @zivkan and @donnie-msft!
There was a problem hiding this comment.
No need to chat, but maintain that the comment is inaccurate.
| /// <summary> | ||
| /// Framework Platform (net5.0+) | ||
| /// </summary> | ||
| public string Platform |
There was a problem hiding this comment.
Suggestion: maybe a different type Platform - to include the name and version?
There was a problem hiding this comment.
Doing it this way makes it consistent with how TFI and TFV are handled, though, so I prefer the split version. Also makes those fields more straightforward to access.
| /// <summary> | ||
| /// True if platform is non-empty (net5.0+) | ||
| /// </summary> | ||
| public bool HasPlatform |
There was a problem hiding this comment.
I know other properties in the class have HasWhatever properties, but I don't like the pattern. Where is it used? Why doesn't the caller do !string.IsNullOrEmpty(PlatformIdentifier) instead?
There was a problem hiding this comment.
I think !string.IsNullOrEmpty() isn't as descriptive as having a boolean to check against that knows the semantics of the value being missing. That said, I'll remove this for now, because I don't know where we'll use it -- I might bring it back if I find a case where it's actually needed for public API purposes.
| /// </summary> | ||
| public bool HasPlatformVersion | ||
| { | ||
| get { return IsNet5Era && !(PlatformVersion.Major == 0 && PlatformVersion.Minor == 0); } |
There was a problem hiding this comment.
Also, what's wrong will null? This is a valid use for null values. It seems wasteful to force the property to be not-null, and then invent our own convention for how to represent "not specified"
| _frameworkProfile = IsNet5Era ? profileOrPlatform ?? string.Empty : string.Empty; | ||
| Platform = IsNet5Era ? profileOrPlatform ?? string.Empty : string.Empty; | ||
| PlatformVersion = NormalizeVersion(platformVersion ?? FrameworkConstants.EmptyVersion); |
There was a problem hiding this comment.
why empty values to represent values that aren't defined? It's a perfect use-case for null. I feel like I'm missing information about why _frameworkProfile already does this.
There was a problem hiding this comment.
I agree, but this is what @nkolev92 pushed for, and I ended up conceding. Happy to jump on a call if you want to bring back the discussion. fwiw, I agree that this is a good use-case for null, in general.
There was a problem hiding this comment.
For reference :)
#9382 (comment)
| ## Remaining work/questions | ||
|
|
||
| 1. Ensure Pack target passes TargetPlatformVersion to .nuspec generator. (Use `0.0` as a sentinel) | ||
| 1. Fail parsing/creation of `NuGetFramework` when an unknown OS flavor is being used -- slam an Unsupported tfi into it?? |
There was a problem hiding this comment.
I'm not sure. I'd like others opinions. In a future .NET, if they want to support new platforms, should they be dependant on NuGet updating our platform whitelist, before they can even progress on other parts of the build system? I'm happy for NuGetFramework's IsSupported to return false if it's not a whitelisted platform, but I just don't know how I feel about returning the unsupported TFI.
|
Hello! I'm working to cleanup stale PRs on Actions to take:
If there's anything I can do to help out, please let me know! |
|
I am closing this PR as it may be abandoned with no reviews since. If it's important to include in the repository, please re-open the PR & assign reviewers. |
Ref: #9240