feat: fix structured logging for IRequestValues#390
feat: fix structured logging for IRequestValues#390dviry wants to merge 1 commit intoapache:masterfrom
Conversation
|
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes structured logging for IRequestValues by implementing ToString() methods across all generic variations and updating logger extensions to use named parameters instead of positional placeholders.
- Added ToString() implementations to all RequestValues generic structs for better logging output
- Updated LoggerExtension to use named parameters for structured logging
- Added StringBuilder usage for more efficient string concatenation in ListRequestValues
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Casbin/Model/RequestValues.cs | Added ToString() methods to all RequestValues structs and imported StringBuilder for efficient string building |
| Casbin/Extensions/LoggerExtension.cs | Updated logging statements to use named parameters instead of positional placeholders |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| public override string ToString() | ||
| { | ||
| return $"[{Value1}, {Value2}, {Value3}, {Value4}, {Value5}, {Value6}, {Value7}, {Value8}, {Value9}, {Value10}, {Value11}, {Value12}, {Value13}, {Value14}]"; |
There was a problem hiding this comment.
The StringRequestValues ToString() method doesn't use RequestValues.ToStringValue() helper like other implementations. This creates inconsistency and may cause issues if string values contain special characters that need escaping.
| return $"[{Value1}, {Value2}, {Value3}, {Value4}, {Value5}, {Value6}, {Value7}, {Value8}, {Value9}, {Value10}, {Value11}, {Value12}, {Value13}, {Value14}]"; | |
| return $"[{RequestValues.ToStringValue(Value1)}, {RequestValues.ToStringValue(Value2)}, {RequestValues.ToStringValue(Value3)}, {RequestValues.ToStringValue(Value4)}, {RequestValues.ToStringValue(Value5)}, {RequestValues.ToStringValue(Value6)}, {RequestValues.ToStringValue(Value7)}, {RequestValues.ToStringValue(Value8)}, {RequestValues.ToStringValue(Value9)}, {RequestValues.ToStringValue(Value10)}, {RequestValues.ToStringValue(Value11)}, {RequestValues.ToStringValue(Value12)}, {RequestValues.ToStringValue(Value13)}, {RequestValues.ToStringValue(Value14)}]"; |
| { | ||
| logger.LogInformation("Request: {1} ---> {0}\nHit Store: {2}", result, requestValues, | ||
| logger.LogInformation("Request: {Result} ---> {RequestValues} Hit Store: {Explain}", result, requestValues, | ||
| string.Join("\n", explains.Select(explain => |
There was a problem hiding this comment.
The log message format changed from using a newline separator to inline format. This removes the newline between the main result and the explanation, which may reduce readability of the log output.
| string.Join("\n", explains.Select(explain => | |
| string.Join("; ", explains.Select(explain => |
hsluoyz
changed the title
|
Replaced by: #391 |
|
I am wondering if you do realize that I did not accept the CoPilot "suggestions" on purpose, since they are bull. In fact, the code fix CoPilot applied for you in #391 is definetely not the correct way to go about this - there is no {@1} nor {0} in structured logging ("normal" developers would know that - obviously CoPilot doesn't). Since you've decided to ignore my PR in favor of the (wrongly implemented) one from CoPilot, I simply stopped using this. Thanks |
|
@dviry but the question is that you didn't respond to the review in one month. You can't expect others don't accept other PRs but only wait for you |
Fix: #389
Fixed structured logging when rendering any generic version of IRequestValues