Skip to content
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.

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

Refactor LogWithNameAttribute #103

Merged
merged 2 commits into from
Feb 27, 2024
Merged

Refactor LogWithNameAttribute #103

merged 2 commits into from
Feb 27, 2024

Conversation

sungam3r
Copy link
Member

No description provided.

@github-actions github-actions bot added documentation Improvements or additions to documentation tests Pull request that adds new or changes existing tests labels Jan 28, 2024
@@ -38,24 +38,7 @@ public LogWithNameAttribute(string newName)
/// <inheritdoc/>
public bool TryCreateLogEventProperty(string name, object? value, ILogEventPropertyValueFactory propertyValueFactory, [NotNullWhen(true)] out LogEventProperty? property)
{
var propValue = propertyValueFactory.CreatePropertyValue(value);

LogEventPropertyValue? logEventPropVal = propValue switch
Copy link
Member Author

Choose a reason for hiding this comment

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

@nblumhardt What was the point to copy objects in that switch block? It looks strange and method should work without it if I have not missed unobvious details. Also I removed logEventPropVal null-check since propertyValueFactory.CreatePropertyValue always returns some not null LogEventPropertyValue.

Copy link
Member Author

Choose a reason for hiding this comment

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

@paymanx Could you please shed light on what is happening in the code here as an author? What is the reason for re-boxing of values?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi guys. Since I did not receive feedback for a month, I merge this and go on with publishing release. If there are errors, then I will release a fix thereafter.

@sungam3r sungam3r added performance Issue or pull request about performance problems and removed documentation Improvements or additions to documentation labels Jan 28, 2024
@sungam3r sungam3r requested a review from nblumhardt January 28, 2024 08:16
@sungam3r
Copy link
Member Author

This PR is the last one to increase code coverage to 100%.

Copy link

codecov bot commented Jan 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c0c2812) 96.35% compared to head (7e9fd08) 100.00%.

Additional details and impacted files
@@             Coverage Diff             @@
##           master      #103      +/-   ##
===========================================
+ Coverage   96.35%   100.00%   +3.64%     
===========================================
  Files          11        11              
  Lines         247       234      -13     
  Branches       36        34       -2     
===========================================
- Hits          238       234       -4     
+ Misses          7         0       -7     
+ Partials        2         0       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sungam3r sungam3r merged commit 5fb68fe into master Feb 27, 2024
8 checks passed
@sungam3r sungam3r deleted the logname branch February 27, 2024 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Issue or pull request about performance problems tests Pull request that adds new or changes existing tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant