Make IResults types public by brunolins16 · Pull Request #40704 · dotnet/aspnetcore

brunolins16 · 2022-03-14T22:07:12Z

halter73 · 2022-03-18T18:19:17Z

    public async Task ExecuteAsync(HttpContext httpContext)
    {
-        var logger = httpContext.RequestServices.GetRequiredService<ILogger<ChallengeResult>>();
+        var logger = httpContext.RequestServices.GetRequiredService<ILogger<ChallengeHttpResult>>();


Do we care about changing log categories? I could see that being considered a breaking change. I don't see a huge benefit of changing it other than slightly cleaner code. @BrennanConroy @davidfowl

I don't think this is super important for preview3, but I'd like to clarify our stance on changing log categories in general.

Changing categories is considered a breaking change, example

aspnetcore/src/SignalR/common/Http.Connections/src/Internal/Transports/WebSocketsServerTransport.cs

Lines 41 to 42 in 49270ee

// We create the logger with a string to preserve the logging namespace after the server side transport renames.

_logger = loggerFactory.CreateLogger("Microsoft.AspNetCore.Http.Connections.Internal.Transports.WebSocketsTransport");

So, i think we should not change the categories. I have pushed a commit following @BrennanConroy example.

@halter73 / @BrennanConroy can you do a quick review? 00877b5

halter73 · 2022-03-18T18:28:42Z

+    /// <summary>
+    /// Gets the Content-Type header for the response.
+    /// </summary>
+    public string ContentType { get; internal set; }


nbd, but why internal set; instead of internal init; or just get;?

Co-authored-by: Brennan <brecon@microsoft.com>

…m/brunolins16/aspnetcore into brunolins16/iresult-apisuggestion

terryaney · 2023-01-25T21:54:54Z

What version is this going to be released in? 8?

ghost · 2023-01-25T21:54:59Z

Hi @terryaney. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

BrennanConroy · 2023-01-25T21:57:03Z

brunolins16 added 9 commits March 9, 2022 09:38

StatuCodeResult as base class

0ab4de9

StatuCodeResult as base class

813786e

Making types public and renaming to HTTPResult

811c604

Update comments

5d66c24

Make setters internal

acf0a3b

Updating public API file

9ddf586

Removing base classes

9a79b02

Changing method name to WriteResultAsStatusCode

a1fef75

Updating API

1ee3138

ghost added the area-runtime label Mar 14, 2022

brunolins16 added 2 commits March 14, 2022 15:07

code cleanup

2c029a8

fixing public api

4103a0f

brunolins16 added old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels and removed area-runtime labels Mar 15, 2022

Simplyfing interfaces

5fdebc4

Kahbazi reviewed Mar 16, 2022

View reviewed changes

Comment thread src/Http/Http.Results/src/HttpResultsWriter.cs Outdated

Removing interfaces + Adding ctors

6146e4a

brunolins16 requested a review from a team March 18, 2022 07:37

brunolins16 marked this pull request as ready for review March 18, 2022 07:38

brunolins16 requested review from BrennanConroy, Tratcher and halter73 as code owners March 18, 2022 07:38

BrennanConroy reviewed Mar 18, 2022

View reviewed changes

Comment thread src/Http/Http.Results/src/AcceptedAtRouteHttpResult.cs Outdated

brunolins16 added 7 commits March 18, 2022 09:54

Fixing xml docs

7d89ce3

Making EmptyResult public

6171f15

Updating JsonHttpResult

6a807cf

Adding missing ctor

5fa233d

Adding missing ctor

dcf745b

Adding missing ctor

3d01bb6

Adding misssed xml comment

e05b04f

halter73 reviewed Mar 18, 2022

View reviewed changes

Comment thread src/Http/Http.Results/src/AcceptedAtRouteHttpResult.cs Outdated

halter73 reviewed Mar 18, 2022

View reviewed changes

Comment thread src/Http/Http.Results/src/AcceptedAtRouteHttpResult.cs Outdated

halter73 reviewed Mar 18, 2022

View reviewed changes

Removing configureResponseHeader

9c628aa

halter73 reviewed Mar 18, 2022

View reviewed changes

Comment thread src/Http/Http.Results/src/JsonHttpResult.cs Outdated

brunolins16 added 2 commits March 18, 2022 13:05

PR Feedback

eada9b4

Moving ctors to internal

ff3cb76

halter73 approved these changes Mar 18, 2022

View reviewed changes

brunolins16 added 2 commits March 18, 2022 14:52

Create logger using string category

00877b5

Adding more unittests

825f4c0

BrennanConroy reviewed Mar 18, 2022

View reviewed changes

Comment thread src/Http/Http.Results/src/ContentHttpResult.cs Outdated

BrennanConroy reviewed Mar 18, 2022

View reviewed changes

Comment thread src/Http/Http.Results/src/EmptyHttpResult.cs Outdated

brunolins16 and others added 2 commits March 18, 2022 15:40

Update src/Http/Http.Results/src/EmptyHttpResult.cs

73800b9

Co-authored-by: Brennan <brecon@microsoft.com>

Update ContentHttpResult.cs

8fb15dd

BrennanConroy approved these changes Mar 18, 2022

View reviewed changes

BrennanConroy reviewed Mar 18, 2022

View reviewed changes

Comment thread src/Http/Http.Results/src/ObjectHttpResult.cs

brunolins16 added 2 commits March 18, 2022 16:49

avoid closure allocation

dac8711

Merge branch 'brunolins16/iresult-apisuggestion' of https://github.co…

4a50104

…m/brunolins16/aspnetcore into brunolins16/iresult-apisuggestion

brunolins16 enabled auto-merge (squash) March 19, 2022 00:13

unit loggerfactory

1d76b6e

brunolins16 merged commit db7d513 into dotnet:main Mar 20, 2022

ghost added this to the 7.0-preview3 milestone Mar 20, 2022

brunolins16 linked an issue Mar 21, 2022 that may be closed by this pull request

Make IResult methods more testable #37502

Closed

brunolins16 mentioned this pull request Mar 21, 2022

Make IResult methods more testable #37502

Closed

brunolins16 deleted the brunolins16/iresult-apisuggestion branch August 2, 2022 20:49

	// We create the logger with a string to preserve the logging namespace after the server side transport renames.
	_logger = loggerFactory.CreateLogger("Microsoft.AspNetCore.Http.Connections.Internal.Transports.WebSocketsTransport");

Conversation

brunolins16 commented Mar 14, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

halter73 Mar 18, 2022

Choose a reason for hiding this comment

Uh oh!

halter73 Mar 18, 2022

Choose a reason for hiding this comment

Uh oh!

BrennanConroy Mar 18, 2022

Choose a reason for hiding this comment

Uh oh!

brunolins16 Mar 18, 2022

Choose a reason for hiding this comment

Uh oh!

halter73 Mar 18, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

terryaney commented Jan 25, 2023

Uh oh!

ghost commented Jan 25, 2023

Uh oh!

BrennanConroy commented Jan 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants