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

Clarify description of With function #1096

Merged
merged 1 commit into from
Apr 25, 2023
Merged

Conversation

tomwheeler
Copy link
Contributor

I found the previous wording confusing, as the use of "prepend" suggests a position of the key-value pairs that was different than what I observed in the output.

What was changed

I reworded a comment that appears in the generated documentation.

Why?

The comment previously said, "With returns Logger instance that prepend every log entry with keyvals. If logger implements WithLogger it is used, otherwise every log call will be intercepted."

I interpret the use of "prepend" to mean that the key-value pairs I supply will appear first in the output. However, I found that they are the last things to appear. After further experimentation, I found that the KV pairs supplied to With appear after both the KV pairs in the parent logger and the string that I provided in the log statement, although they do appear before any KV pairs included in a specific log statement.

Chad Retz from the SDK team agreed that prepend was not the correct verb to use here, so I have reworded to remove it. As a result, it no longer makes any guarantee about the position of these key-value pairs, but that seemed like an implementation detail, and I find it to be more clear now because I'm no longer misguided about where within the output line I should be looking.

Checklist

  1. Closes N/A

  2. How was this tested:
    I ran go doc --all in the log subdirectory of my sdk-go clone and verified that the comment appeared as I had intended.

  3. Any docs updates needed?

This change updates the only documentation about log.With of which I am aware.

I found the previous wording confusing, as the use of "prepend" suggests a position of the key-value pairs that was different than what I observed in the output.
@tomwheeler tomwheeler requested a review from a team as a code owner April 25, 2023 00:29
@tomwheeler tomwheeler requested a review from cretz April 25, 2023 00:29
@cretz
Copy link
Member

cretz commented Apr 25, 2023

The CI failure is due to bad cloud secret. I am looking into updating now.

@cretz cretz merged commit 5cf9af3 into master Apr 25, 2023
@cretz cretz deleted the tomwheeler-improve-logger-doc branch April 25, 2023 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants