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

fix: inline Attrs for a group with an empty key #27

Merged
merged 3 commits into from
Aug 14, 2023
Merged

fix: inline Attrs for a group with an empty key #27

merged 3 commits into from
Aug 14, 2023

Conversation

telemachus
Copy link
Contributor

This fixes a bug where tint produces .c=d if the user passes an attribute like slog.Group("", slog.String("c", "d")). This result violates one of slog's rules for handlers: "If a group's key is empty, inline the group's Attrs."

This fixes a bug where tint produces `.c=d` if the user passes an
attribute like `slog.Group("", slog.String("c", "d"))`. This result
violates one of slog's rules for handlers: "If a group's key is empty,
inline the group's Attrs."
@telemachus
Copy link
Contributor Author

Full disclosure: I found this bug after testing test/slogtest on tint.

I didn't include my slogtest code in the PR because (frankly) it's gross. slogtest requires you to write a (basic) parser for your output, and that's a bit difficult to do with handlers like tint that produce less fully structured output. (I discovered this while working on my own handler.)

In any case, if you're curious to see an example of slogtest for tint, here's what I used. You're welcome to use that gist however you like (or completely ignore it!).

@lmittmann
Copy link
Owner

Thanks for the fix! I will take a look at slogtest and your gist, might be worth to add a test case using it, since tint aims to keep the output format of slog.TextHandler for all non-"standard-attributes".

@lmittmann lmittmann merged commit d74d683 into lmittmann:main Aug 14, 2023
@telemachus
Copy link
Contributor Author

tint aims to keep the output format of slog.TextHandler for all non-"standard-attributes".

Yup, that's exactly my situation with humane as well.

This seems to be the stumbling block (from slogtest's docs):

The results function is invoked after all such calls. It should return a slice of map[string]any, one for each call to a Logger output method. … The standard keys slog.TimeKey, slog.LevelKey and slog.MessageKey should be used.

Since we've both made our handlers less than fully-structured for the level, time, and message attributes, I wrote something pretty hacky to make slogtest work. I'll be curious to know if you come up with something more elegant.

In theory, maybe slogtest could provide two different modes: one to test everything and one to test only the non-standard attributes. But I'm not sure whether they would accept a PR like that. (Perhaps from their point of view, nearly all slog-conforming handlers will be fully structured and this problem won't arise.)

@telemachus telemachus deleted the empty-group-fix branch August 14, 2023 14:01
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.

2 participants