Skip to content

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

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
merged 3 commits into from
Jul 6, 2021
Merged

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

merged 3 commits into from
Jul 6, 2021

Conversation

maryamariyan
Copy link
Contributor

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

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

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

Once the dependency PR comes in yea.

This was referenced Jul 12, 2021
Open
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