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

Add Name() function to Logger struct #1273

Merged
merged 3 commits into from
May 7, 2023

Conversation

rexywork
Copy link
Contributor

Related to #1200

This PR add Name() function to Logger struct which will return the Logger's Name.
By default, Logger's name is empty if not set so for Unnamed Logger it will return empty string.

Please let me know which file test that I should include for testing this function and I will include the unit test.
Thank you.

@CLAassistant
Copy link

CLAassistant commented Apr 17, 2023

CLA assistant check
All committers have signed the CLA.

@rexywork rexywork mentioned this pull request Apr 17, 2023
Copy link
Collaborator

@abhinav abhinav left a comment

Choose a reason for hiding this comment

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

logger.go Outdated Show resolved Hide resolved
Copy link
Member

@mway mway left a comment

Choose a reason for hiding this comment

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

lgtm

@codecov
Copy link

codecov bot commented May 6, 2023

Codecov Report

Merging #1273 (13d4936) into master (845ca51) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1273   +/-   ##
=======================================
  Coverage   98.08%   98.08%           
=======================================
  Files          50       50           
  Lines        3240     3242    +2     
=======================================
+ Hits         3178     3180    +2     
  Misses         53       53           
  Partials        9        9           
Impacted Files Coverage Δ
logger.go 96.55% <100.00%> (+0.03%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Co-authored-by: Abhinav Gupta <mail@abhinavg.net>
Copy link
Contributor

@sywhang sywhang left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

@rexywork
Copy link
Contributor Author

rexywork commented May 7, 2023

@abhinav @sywhang @mway Seems I need to add test for the Name() function to pass the coverage.
After looking the test, I found func TestLoggerNames(t *testing.T) here: https://github.com/uber-go/zap/blob/master/logger_test.go#L319
Should we just adjust this test? For example, in here and here, we can change:
logs.AllUntimed()[0].LoggerName to log.Name() or log.base.Name() for SugaredLogger (Instead of getting the LoggerName from the Entry, we can get directly from Logger variables)
What do you think? If you agree, I can push new changes. Thank you

@mway
Copy link
Member

mway commented May 7, 2023

Instead of replacing (because we still want to validate that entries have the correct logger name), just adding a second check against the expected logger name should be fine.

Thank you!

@rexywork
Copy link
Contributor Author

rexywork commented May 7, 2023

@mway Ah I see, I will add a line check in the same test function since it kind of weird to separate as new test function.
Thank you for the quick response!

Adjust error messages for the test that check Logger Names from Entry.
@rexywork
Copy link
Contributor Author

rexywork commented May 7, 2023

@mway @abhinav @sywhang I already adjust the test and the coverage check already passed.
Please help me review and kindly help to merge if there are no additional request. Thank you

@mway mway merged commit 24b7977 into uber-go:master May 7, 2023
@mway
Copy link
Member

mway commented May 7, 2023

Thanks @rexywork!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants