-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Contributing: fixed some typos #18494
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
Conversation
@@ -30,7 +30,7 @@ public partial class ConnectionOptions | |||
} | |||
public partial class ConnectionOptionsSetup : Microsoft.Extensions.Options.IConfigureOptions<Microsoft.AspNetCore.Http.Connections.ConnectionOptions> | |||
{ | |||
public static System.TimeSpan DefaultDisconectTimeout; | |||
public static System.TimeSpan DefaultDisconnectTimeout; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well that's unfortunate...
We can't really change that arbitrarily
cc @anurse
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, unfortunately it's a breaking change. That doesn't mean we can't take it though :). Let's raise this in an API review. I'll set things up for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently it isn't readonly
.
API Review SummaryWe have a typo in a public API. The API is a The disconnect timeout is the amount of time a long-polling connection can be idle for before it will be considered terminated. Since there's no functional change (it's just a typo in the name) I'm not really sure this meets the bar for breaking changes, but it does suck that we have a typo here :(. The scenarios for consuming this API are fairly minimal. It basically just allows someone to access the original default value even after the value has changed. |
@anurse First, thank you for your precious feedback. I totally agree with you that this change doesn't meet the bar for breaking changes but it could be if you wish an enhancement. I also understand that is not internal but public API. I suggest discarding that change for now. |
@anurse as @campersau said, it is not readonly yet for the DefaultDisconectTimeout and it should be. |
@@ -30,7 +30,7 @@ public partial class ConnectionOptions | |||
} | |||
public partial class ConnectionOptionsSetup : Microsoft.Extensions.Options.IConfigureOptions<Microsoft.AspNetCore.Http.Connections.ConnectionOptions> | |||
{ | |||
public static System.TimeSpan DefaultDisconectTimeout; | |||
public static readonly System.TimeSpan DefaultDisconectTimeout; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also a breaking change. In general, anything that modifies code in a ref/
directory will be a breaking change. I'm a little disappointed this wasn't readonly
to begin with but again, we need to be cautious here.
That doesn't mean we can't take it! It just means we have to get some consensus among the team about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we obsolete the API? This probably should have been internal to begin with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's one option. We could just phase it out starting in 5.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave this be. We don't think it's worth while obsoleting or changing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still needs to be reverted.
@MaherJendoubi I'll bring this up with some other folks on the team to see how they feel (since I don't want to speak unilaterally for the team). I'd like to feel like we can fix typos like this eventually, but of course, we have to balance that against the desire to avoid breaking changes that don't bring value. The change to make the field readonly (while absolutely correct) is also breaking so we should have a quick conversation about it within the team. |
@@ -28,9 +28,9 @@ public partial class ConnectionOptions | |||
public ConnectionOptions() { } | |||
public System.TimeSpan? DisconnectTimeout { [System.Runtime.CompilerServices.CompilerGeneratedAttribute] get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute] set { } } | |||
} | |||
public partial class ConnectionOptionsSetup : Microsoft.Extensions.Options.IConfigureOptions<Microsoft.AspNetCore.Http.Connections.ConnectionOptions> | |||
internal partial class ConnectionOptionsSetup : Microsoft.Extensions.Options.IConfigureOptions<Microsoft.AspNetCore.Http.Connections.ConnectionOptions> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would need to be undone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted that change.
@@ -30,7 +30,7 @@ public partial class ConnectionOptions | |||
} | |||
public partial class ConnectionOptionsSetup : Microsoft.Extensions.Options.IConfigureOptions<Microsoft.AspNetCore.Http.Connections.ConnectionOptions> | |||
{ | |||
public static System.TimeSpan DefaultDisconectTimeout; | |||
public static readonly System.TimeSpan DefaultDisconectTimeout; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave this be. We don't think it's worth while obsoleting or changing this.
This reverts commit 90f94d2.
@@ -44,7 +44,7 @@ internal static class Log | |||
LoggerMessage.Define<HttpTransportType>(LogLevel.Trace, new EventId(11, "ReceivedDeleteRequestForUnsupportedTransport"), "Received DELETE request for unsupported transport: {TransportType}."); | |||
|
|||
private static readonly Action<ILogger, Exception> _terminatingConnection = | |||
LoggerMessage.Define(LogLevel.Trace, new EventId(12, "TerminatingConection"), "Terminating Long Polling connection due to a DELETE request."); | |||
LoggerMessage.Define(LogLevel.Trace, new EventId(12, "TerminatingConnection"), "Terminating Long Polling connection due to a DELETE request."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Log IDs can't be changed, they're effectively public.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have just reverted it.
@@ -8,7 +8,7 @@ namespace Microsoft.AspNetCore.Http.Connections | |||
{ | |||
public class ConnectionOptionsSetup : IConfigureOptions<ConnectionOptions> | |||
{ | |||
public static TimeSpan DefaultDisconectTimeout = TimeSpan.FromSeconds(15); | |||
public static readonly TimeSpan DefaultDisconectTimeout = TimeSpan.FromSeconds(15); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert
src/SignalR/common/Http.Connections/ref/Microsoft.AspNetCore.Http.Connections.netcoreapp.cs
Outdated
Show resolved
Hide resolved
src/SignalR/common/Http.Connections/src/ConnectionOptionsSetup.cs
Outdated
Show resolved
Hide resolved
Hello @BrennanConroy! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
Thanks for the contribution @MaherJendoubi ! |
deleted some extra spaces
fixed some typos