Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add public API baselines to SignalR #25653

Merged
merged 1 commit into from
Sep 10, 2020
Merged

Add public API baselines to SignalR #25653

merged 1 commit into from
Sep 10, 2020

Conversation

Pilchie
Copy link
Member

@Pilchie Pilchie commented Sep 6, 2020

No description provided.

@ghost ghost added the area-signalr Includes: SignalR clients and servers label Sep 6, 2020
@Pilchie Pilchie added this to the 5.0.0-rc2 milestone Sep 6, 2020
@Pilchie Pilchie added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Sep 6, 2020
@Pilchie
Copy link
Member Author

Pilchie commented Sep 6, 2020

Part of #24347

@dougbu dougbu mentioned this pull request Sep 6, 2020
Closed
dougbu
dougbu approved these changes Sep 6, 2020
View reviewed changes
src/SignalR/server/Core/src/HubConnectionContext.cs
@@ -146,6 +147,7 @@ internal StreamTracker StreamTracker
// Currently used only for streaming methods
internal ConcurrentDictionary<string, CancellationTokenSource> ActiveRequestCancellationSources { get; } = new ConcurrentDictionary<string, CancellationTokenSource>(StringComparer.Ordinal);

[SuppressMessage("ApiDesign", "RS0026:Do not add multiple overloads with optional parameters", Justification = "Required to maintain compatibility")]
Copy link
Member

Choose a reason for hiding this comment

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

nit: Should we craft a better Justification for the many T1 parameter, CancellationToken token = default, T2 parameter, CancellationToken token = default, … cases❔ If we were adding HubConnectionContext tomorrow, I bet we'd use the same set of overloads. Perhaps,

Suggested change
[SuppressMessage("ApiDesign", "RS0026:Do not add multiple overloads with optional parameters", Justification = "Required to maintain compatibility")]
[SuppressMessage("ApiDesign", "RS0026:Do not add multiple overloads with optional parameters", Justification = "May need a CancellationToken with either of these message types.")]

Of course, if you do that here, we'd need to look at the rest of our open PRs and I'm not sure it's worthwhile.

@dougbu
Copy link
Member

dougbu commented Sep 6, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@Pilchie
Copy link
Member Author

Pilchie commented Sep 9, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@Pilchie Pilchie removed this from the 5.0.0-rc2 milestone Sep 10, 2020
@Pilchie Pilchie added auto-merge tell-mode Indicates a PR which is being merged during tell-mode labels Sep 10, 2020
@ghost
Copy link

ghost commented Sep 10, 2020

Hello @Pilchie!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

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 (@msftbot) and give me an instruction to get started! Learn more here.

@ghost
Copy link

ghost commented Sep 10, 2020

Apologies, while this PR appears ready to be merged, it looks like release/5.0-rc2 is a protected branch and I have not been granted permission to perform the merge.

@hoyosjs hoyosjs mentioned this pull request Sep 10, 2020
Merged
@Pilchie Pilchie merged commit 158401f into dotnet:release/5.0-rc2 Sep 10, 2020
@Pilchie Pilchie deleted the api-baselines-signalr branch September 10, 2020 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dougbu dougbu dougbu approved these changes

@BrennanConroy BrennanConroy Awaiting requested review from BrennanConroy

@halter73 halter73 Awaiting requested review from halter73

@davidfowl davidfowl Awaiting requested review from davidfowl

Assignees
No one assigned
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework area-signalr Includes: SignalR clients and servers tell-mode Indicates a PR which is being merged during tell-mode
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Pilchie @dougbu