Skip to content

Conversation

@tarekgh
Copy link
Member

@tarekgh tarekgh commented Jun 18, 2023

Fixes #87254

@ghost
Copy link

ghost commented Jun 18, 2023

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, 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 18, 2023

Tagging subscribers to this area: @dotnet/area-extensions-logging
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #87254

Author: tarekgh
Assignees: tarekgh
Labels:

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

Milestone: -

@tarekgh tarekgh added this to the 8.0.0 milestone Jun 18, 2023
@tarekgh
Copy link
Member Author

tarekgh commented Jun 18, 2023

CC @noahfalk @geeknoid

@geeknoid
Copy link
Member

Note that this change might require an update to the dotnet/runtime logging generator (distinct from the dotnet/extensions extended generator)

@tarekgh
Copy link
Member Author

tarekgh commented Jun 18, 2023

Note that this change might require an update to the dotnet/runtime logging generator (distinct from the dotnet/extensions extended generator)

What do you mean by require an update to the dotnet/runtime logging generator? The changes here already include the changes in the runtime generator.

@geeknoid
Copy link
Member

Note that this change might require an update to the dotnet/runtime logging generator (distinct from the dotnet/extensions extended generator)

What do you mean by require an update to the dotnet/runtime logging generator? The changes here already include the changes in the runtime generator.

Duh. Never mind, I spaced out...

@tarekgh tarekgh merged commit eb71494 into dotnet:main Jun 18, 2023
@tarekgh tarekgh deleted the LoggerMessageAttributeNewConstructors branch June 18, 2023 22:20
eventId = items[0].IsNull ? -1 : (int)GetItem(items[0]);
level = items[1].IsNull ? null : (int?)GetItem(items[1]);
message = items[2].IsNull ? string.Empty : (string)GetItem(items[2]);
switch (items.Length)
Copy link
Member

Choose a reason for hiding this comment

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

Do we correctly handle, e.g.

[LoggerMessage(level: LogLevel.Warning, message: "custom message", eventId: 0)]

?

Copy link
Member Author

Choose a reason for hiding this comment

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

I confirmed this work with and without the new changes.

@geeknoid
Copy link
Member

So @tarekgh, we should make sure both generators do this the same way. So please let me know what hash function you end up using on the event name in order to produce the event id, and I'll use the same thing on my end.

@ghost ghost locked as resolved and limited conversation to collaborators Jul 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add additional LoggerMessageAttribute constructors

3 participants