Skip to content

Conversation

@Anipik Anipik requested review from leecow and rbhanda as code owners May 14, 2021 18:01
Copy link
Contributor

@pranavkm pranavkm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me


``` diff
namespace Microsoft.AspNetCore.Builder {
+ public sealed class Configuration : IConfiguration, IConfigurationBuilder, IConfigurationRoot {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@halter73 could you verify the minimal-host changes?

# System.Numerics

``` diff
namespace System.Numerics {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

System.Numerics changes LGTM.

CC. @pgovind

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Numerics LGTM


``` diff
namespace System {
- public readonly struct Byte : IComparable, IComparable<byte>, IConvertible, IEquatable<byte>, IFormattable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ISpanFormattable addition to core numeric types LGTM.

CC. @pgovind, @stephentoub


``` diff
+namespace Microsoft.AspNetCore.HttpLogging {
+ public enum HttpLoggingFields : long {
Copy link

@svengeance svengeance May 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW the generated diffs here end up with the calculated values - I was confused as to why this wasn't done by bitwise-OR'ing fields together, and it turns out that it is done this way, but you can only see it in the source source.dot.net .

It would be good to have the diffs generated, if possible, with the broken-out values to make it easier to understand rather than the final bitwise calculation

cc @tannergooding - discussed with him on the C# Discord

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notably the diff here is also missing the [Flags] attribute that is in the source/ref assembly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i will take a look if we can fix this issue in the tooling.

@tarekgh
Copy link
Member

tarekgh commented May 14, 2021

Date/Time/Time zones included in System namespace changes LGTM.

# Microsoft.AspNetCore.HttpLogging

``` diff
+namespace Microsoft.AspNetCore.HttpLogging {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Anipik
Copy link
Contributor Author

Anipik commented May 17, 2021

hello everyone, can everyone validate their area changes by end of the day.

@@ -0,0 +1,142 @@
# System.Text.Json.Node
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JsonNode diff looks good.

``` diff
namespace System.Threading {
public class CancellationTokenSource : IDisposable {
+ public bool TryReset();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

System.Threading change LGTM

# System.Numerics

``` diff
namespace System.Numerics {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Numerics LGTM

Copy link

@layomia layomia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

System.Text.Json LGTM

Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM from a Linq & Json perspective.

@Anipik Anipik merged commit f98ed72 into dotnet:main May 18, 2021
@Anipik Anipik deleted the preview4 branch May 18, 2021 16:18
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.