Skip to content

Create PublicAPI.Unshipped.txt #24299

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
Merged
2 commits merged into from
Jul 25, 2020
Merged

Create PublicAPI.Unshipped.txt #24299

2 commits merged into from
Jul 25, 2020

Conversation

TanayParikh
Copy link
Contributor

@TanayParikh TanayParikh commented Jul 24, 2020 •
edited
Loading

Fixes: #24258 (comment)

Still getting RS0026 (“Do not add multiple public overloads with optional parameters”)

Extensions\HttpResponseWritingExtensions.cs(26,28): error RS0026: Symbol 'WriteAsync' violates the backcompat requirement: 'Do not add multiple overloads with optional parameters'. See 'https://github.com/dotnet/roslyn/blob/master/docs/Adding%20Optional%20Parameters%20in%20Public%20API.md' for details. [C:\aspnetcore\src\Http\Http.Abstractions\src\Microsoft.AspNetCore.Http.Abstractions.csproj]
Extensions\HttpResponseWritingExtensions.cs(49,28): error RS0026: Symbol 'WriteAsync' violates the backcompat requirement: 'Do not add multiple overloads with optional parameters'. See 'https://github.com/dotnet/roslyn/blob/master/docs/Adding%20Optional%20Parameters%20in%20Public%20API.md' for details. [C:\aspnetcore\src\Http\Http.Abstractions\src\Microsoft.AspNetCore.Http.Abstractions.csproj]

@ghost ghost added the area-servers label Jul 24, 2020
@TanayParikh TanayParikh requested a review from dougbu July 24, 2020 21:02
@TanayParikh
Copy link
Contributor Author

@dougbu is this the intended resolution to #24258 (comment)?

@TanayParikh
Copy link
Contributor Author

Hmm, CI not passing, going through it again.

Wasn't getting RS0016 locally with these changes.

@dougbu
Copy link
Contributor

dougbu commented Jul 24, 2020

No, get rid of the src/Http files and you'll be done. My setup was an example for adding a brand new API baseline. Sorry if I didn't make that clear.

The updated existing baseline files look good.

dougbu
dougbu reviewed Jul 24, 2020
View reviewed changes
src/Mvc/Mvc.Abstractions/src/PublicAPI.Unshipped.txt Outdated
@@ -51,7 +51,7 @@ Microsoft.AspNetCore.Mvc.ActionConstraints.ActionConstraintContext.Candidates.ge
Microsoft.AspNetCore.Mvc.ActionConstraints.ActionConstraintContext.Candidates.set -> void
Microsoft.AspNetCore.Mvc.ActionConstraints.ActionConstraintContext.CurrentCandidate.get -> Microsoft.AspNetCore.Mvc.ActionConstraints.ActionSelectorCandidate
Microsoft.AspNetCore.Mvc.ActionConstraints.ActionConstraintContext.CurrentCandidate.set -> void
Microsoft.AspNetCore.Mvc.ActionConstraints.ActionConstraintContext.RouteContext.get -> Microsoft.AspNetCore.Routing.RouteContext!
Microsoft.AspNetCore.Mvc.ActionConstraints.ActionConstraintContext.RouteContext.get -> RouteContext!
Copy link
Contributor

Choose a reason for hiding this comment

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

On second thought, I'm not sure why these lines are changing. Suggest running through the update steps I described in email using src/Mvc/startvs.cmd instead. Something may be missing.

@dougbu
Copy link
Contributor

dougbu commented Jul 24, 2020

Wasn't getting RS0016 locally with these changes.

In more detail, this was my fault for not making the example clear. Using src/Mvc/startvs.cmd I'd expect a little more to change e.g. in Mvc.Core's API baselines.

@TanayParikh
Copy link
Contributor Author

Retrying, thanks

@dougbu
Copy link
Contributor

dougbu commented Jul 24, 2020

Thanks @TanayParikh /fyi the RS0026 errors in src/Http are expected and need to be suppressed. But, let's not mix adding new API baselines into this PR.

@TanayParikh TanayParikh force-pushed the taparik/add_public_apis branch from 3fd32a1 to 602afec Compare July 24, 2020 22:13
@TanayParikh TanayParikh requested a review from dougbu July 24, 2020 22:15
dougbu
dougbu approved these changes Jul 24, 2020
View reviewed changes
Copy link
Contributor

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

These changes look on-target. @pranavkm you might want to refer to this PR in your API Review 😺

@ghost
Copy link

ghost commented Jul 24, 2020

Hello @TanayParikh!

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.

@TanayParikh TanayParikh mentioned this pull request Jul 24, 2020
Merged
@pranavkm
Improve record type detection
88df305 
Support both forms of Clone methods to detect record types
dougbu
dougbu reviewed Jul 24, 2020
View reviewed changes
src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultBindingMetadataProvider.cs
@@ -143,7 +143,8 @@ private static ConstructorInfo GetRecordTypeConstructor(Type type, ConstructorIn
static bool IsRecordType(Type type)
{
// Based on the state of the art as described in https://github.com/dotnet/roslyn/issues/45777
var cloneMethod = type.GetMethod("<>Clone", BindingFlags.Public | BindingFlags.Instance);
var cloneMethod = type.GetMethod("<Clone>$", BindingFlags.Public | BindingFlags.Instance) ??
type.GetMethod("<>Clone", BindingFlags.Public | BindingFlags.Instance);
Copy link
Contributor

Choose a reason for hiding this comment

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

Accidentally commented on a commit:

Are we using a slightly different version of Roslyn in 'master'❔
And, how did validation pass for the merge PR from preview8 (https://github.com/dotnet/aspnetcore/pull/24258/checks)❔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc/ @pranavkm

@ghost ghost merged commit b094ecd into master Jul 25, 2020
@ghost ghost deleted the taparik/add_public_apis branch July 25, 2020 00:45
@dougbu
Copy link
Contributor

dougbu commented Jul 25, 2020

Yippee 🥇

@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023
This pull request was closed.
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

@Pilchie Pilchie Pilchie approved these changes

@jkotalik jkotalik Awaiting requested review from jkotalik

@Tratcher Tratcher Awaiting requested review from Tratcher

Assignees
No one assigned
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@TanayParikh @dougbu @pranavkm @Pilchie @AlekseyTs @jcouv @amcasey