Skip to content

Update new LoggerMessage.Define APIs, to take LogDefineOptions#54581

Merged
maryamariyan merged 3 commits intodotnet:mainfrom
maryamariyan:define-overloads
Jul 6, 2021
Merged

Update new LoggerMessage.Define APIs, to take LogDefineOptions#54581
maryamariyan merged 3 commits intodotnet:mainfrom
maryamariyan:define-overloads

Conversation

@maryamariyan
Copy link
Contributor

@maryamariyan maryamariyan commented Jun 22, 2021

Change API for the new LoggerMessage.Define overloads, to take LogDefineOptions

Contributes to #50913

@ghost
Copy link

ghost commented Jun 22, 2021

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Jun 22, 2021

Tagging subscribers to this area: @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

Change API for the new LoggerMessage.Define overloads, to take LogDefineOptions

Contributes to #50913

Author: maryamariyan
Assignees: -
Labels:

area-Extensions-Logging, new-api-needs-documentation
Milestone: -

@maryamariyan maryamariyan requested a review from eerhardt June 22, 2021 22:21
eerhardt
eerhardt reviewed Jun 23, 2021
View reviewed changes
@maryamariyan maryamariyan changed the title Update new API LoggerMessage.Define APIs, to take LogDefineOptions Update new API LoggerMessage.Define APIs, to take LogDefineOptions Jun 23, 2021
@maryamariyan maryamariyan changed the title Update new API LoggerMessage.Define APIs, to take LogDefineOptions Update new LoggerMessage.Define APIs, to take LogDefineOptions Jun 23, 2021
@maryamariyan maryamariyan marked this pull request as draft June 23, 2021 02:30
@maryamariyan maryamariyan marked this pull request as ready for review June 25, 2021 15:46
eerhardt
eerhardt approved these changes Jun 25, 2021
View reviewed changes
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

:shipit:

@maryamariyan maryamariyan merged commit be5a950 into dotnet:main Jul 6, 2021
@davidfowl
Copy link
Member

davidfowl commented Jul 6, 2021

This is gonna be a breaking change when it reaches ASP.NET Core right? Can you do the reaction PR as well @maryamariyan ?

@maryamariyan
Copy link
Contributor Author

maryamariyan commented Jul 6, 2021

This is gonna be a breaking change when it reaches ASP.NET Core right? Can you do the reaction PR as well @maryamariyan ?

ok, so as to update any usages in aspnetcore repo?

@davidfowl
Copy link
Member

davidfowl commented Jul 6, 2021

Once the dependency PR comes in yea.

This was referenced Jul 12, 2021
Closed
Closed
@dougbu dougbu mentioned this pull request Jul 14, 2021
Merged
@dougbu dougbu mentioned this pull request Jul 23, 2021
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Aug 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@eerhardt eerhardt eerhardt approved these changes

Assignees

No one assigned

Labels

area-Extensions-Logging new-api-needs-documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@maryamariyan @davidfowl @eerhardt