Uh oh!
There was an error while loading. Please reload this page.
Api diff between net5.0 and netcoreapp3.1 & netstandard2.1 - #5610
Conversation
Uh oh!
There was an error while loading. Please reload this page.
terrajobst
left a comment
There was a problem hiding this comment.
Please make sure the file paths follow product naming. For example, it's not .Net, it's .NET.
Uh oh!
There was an error while loading. Please reload this page.
carlossanlop
left a comment
There was a problem hiding this comment.
Left a couple of questions, but aside from that, signing off on behalf of System.IO* and System.Threading.AccessControl.
Uh oh!
There was an error while loading. Please reload this page.
AaronRobinsonMSFT
left a comment
There was a problem hiding this comment.
LGTM - System.Runtime.InteropServices and System.Runtime.InteropServices.WindowsRuntime
| ``` diff | ||
| +namespace System.Formats.Asn1 { | ||
| + public readonly struct Asn1Tag : IEquatable<Asn1Tag> { |
There was a problem hiding this comment.
Everything in the System.Formats.Asn1 is in a NuGet package available to .NET Standard 2.0; so it's available with netcoreapp3.1.
On the one hand, I like that people who can benefit from it it can see that it exists. On the other, I'm not sure it really belongs in this diff.
There was a problem hiding this comment.
I suggest we add comments or text at the top of the file to indicate places where APIs were previously available via nuget and are now available in-box.
There was a problem hiding this comment.
I was going to say "but System.Formats.Asn1 isn't in the ref-set for net5.0, it's just in the shared framework as an implementation detail". But, apparently that's not true, it seems to be in the ref-set.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| ``` diff | ||
| namespace System.Security.Cryptography.X509Certificates { | ||
| public sealed class CertificateRequest { | ||
| + public X509Certificate2 Create(X500DistinguishedName issuerName, X509SignatureGenerator generator, DateTimeOffset notBefore, DateTimeOffset notAfter, ReadOnlySpan<byte> serialNumber); |
There was a problem hiding this comment.
I can understand "last release to this release" and "last LTS to this release" (they happen to both be from 3.1 to 5.0 in this case); but why would we publish "from two LTSes ago"? That's just "2.1 -> 3.1 union 3.1 -> 5.0"
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
eiriktsarpalis
left a comment
There was a problem hiding this comment.
Collections and LINQ changes LGTM.
stephentoub
commented
Nov 17, 2020
Why? |
Uh oh!
There was an error while loading. Please reload this page.
Anipik
commented
Nov 17, 2020
@richlander asked me to do this, can you add the reason ? |
Uh oh!
There was an error while loading. Please reload this page.
| @@ -0,0 +1,16 @@ | |||
| # System.Numerics | |||
There was a problem hiding this comment.
Numerics changes LGTM. CC. @pgovind
| @@ -0,0 +1,2340 @@ | |||
| # System.Runtime.Intrinsics.Arm | |||
There was a problem hiding this comment.
ARM intrinsics LGTM. CC @CarolEidt, @echesakovMSFT, @kunalspathak
| @@ -0,0 +1,125 @@ | |||
| # System.Runtime.Intrinsics.X86 | |||
There was a problem hiding this comment.
x86 intrinsics LGTM. CC @CarolEidt, @echesakovMSFT, @kunalspathak
| @@ -0,0 +1,35 @@ | |||
| # System.Runtime.Intrinsics | |||
There was a problem hiding this comment.
General intrinsics LGTM. CC @CarolEidt, @echesakovMSFT, @kunalspathak
Also CC. @pgovind for the Numerics interop APIs
| + public long PromotedBytes { get; } | ||
| + public long TotalCommittedBytes { get; } | ||
| } | ||
| + public readonly struct Half : IComparable, IComparable<Half>, IEquatable<Half>, IFormattable { |
There was a problem hiding this comment.
Half changes LGTM. CC. @pgovind
| + public static bool TryParse(string s, NumberStyles style, IFormatProvider provider, out IntPtr result); | ||
| + public static bool TryParse(string s, out IntPtr result); | ||
| } | ||
| public static class Math { |
There was a problem hiding this comment.
Math changes LGTM. CC. @pgovind
| @@ -0,0 +1,101 @@ | |||
| # System.Drawing.Common | |||
There was a problem hiding this comment.
System.Drawing LGTM. CC @safern
kouvel
commented
Nov 17, 2020
LGTM for System.Threading |
pgovind
left a comment
There was a problem hiding this comment.
Math, Numerics, RegEx and Half looks good
| + public SortKey GetSortKey(string source, CompareOptions options); | ||
| + public int GetSortKeyLength(ReadOnlySpan<char> source, CompareOptions options = CompareOptions.None); | ||
| + public int IndexOf(ReadOnlySpan<char> source, ReadOnlySpan<char> value, CompareOptions options = CompareOptions.None); | ||
| + public int IndexOf(ReadOnlySpan<char> source, ReadOnlySpan<char> value, CompareOptions options, out int matchLength); |
There was a problem hiding this comment.
@GrabYourPitchforks is there any reason why the diff here not included in ns2,1?
| # System.Resources | ||
| ``` diff | ||
| namespace System.Resources { |
tarekgh
commented
Nov 17, 2020
System.Globalization, System.Text, System.Threading.Tasks, System.Threading.Channels, and System.Diagnostics (modulo Process change) looks good to me. |
Anipik
commented
Nov 18, 2020
addressed the feedback, also changed .net -> .NET, asp.net -> ASP.NET, got most of the approvals here as well |
| # System.Collections.Immutable | ||
| ``` diff | ||
| { |
There was a problem hiding this comment.
@Anipik should you delete these files since you removed them from the TOC?
There was a problem hiding this comment.
Table of contents, this file: https://github.com/dotnet/core/pull/5610/files#diff-2226bc7a0ac5271249dcb658c1c9679c1a13e8e0a4e9c04381cdab801784dbb2
There was a problem hiding this comment.
yeah i will check whether these files needs to be removed or we need the entry back in TOC
| ``` diff | ||
| { | ||
| - namespace System.Drawing { |
There was a problem hiding this comment.
Not sure if we want a note, but these were all pushed down into System.Drawing.Conmmon.
There was a problem hiding this comment.
i added a comment for the entire assemblies to be moved down, i will add for these types as well
Uh oh!
There was an error while loading. Please reload this page.
* .net shared framework changes * standalone net 5.0 packages * netstanard2.1 diff * improving the directory structure * adding a readme file * aspnetcore shared framework changes * remove wrong process type change diff * adding comments about apis being to inbox from package
@eiriktsarpalis@layomia@steveharter@jozkee@pgovind@eerhardt@tarekgh@stephentoub
@kouvel @dotnet/ncl @JeremyKuhne@safern@tannergooding@JeremyKuhne@carlossanlop
@AaronRobinsonMSFT@jkoritzinsky@tannergooding@CarolEidt @echesakovMSFT @danmosemsft
@ericstj@richlander
@pranavkm@Tratcher@Pilchie@davidfowl