-
Notifications
You must be signed in to change notification settings - Fork 6
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
Headers, error formatting, multiline attr value handling, dim theme, and ReplaceAttr support #15
Open
ansel1
wants to merge
13
commits into
phsym:main
Choose a base branch
from
ansel1:alltogether
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+953
−97
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Makes attribute values and keys a little darker to make the messages pop more. Addresses issue phsym#9.
Uses fmt.Fprintf with "%+v" to print errors. Useful with some error packages which render additional information like stacktraces. Addresses phsym#7
Sort multiline values to the end, and print the key on a separate line
HandlerOptions.ReplaceAttr support. When ReplaceAttr is set, there will be more allocations. There is one allocation happening even when ReplaceAttr is nil, around passing the slice of groups to ReplaceAttr. Still needs to be optimized away.
Encoders are now the disposable state struct for each Handle call. A sync pool of encoders replaces the sync pool of buffers. This allows the encoder to store additional state per Handle() call, including a slice of currently open groups, which is needed to call ReplaceAttr. Benchmarking shows 0 allocations, regardless of whether ReplaceAttr is set or not. Setting a ReplaceAttr does add some overhead, similar to the slog.TextHandler. Benchmarking also shows a slight performance improvement over the original code (using the buf pool), when ReplaceAttrs is not set.
If the error doesn't implement fmt.Formatter, as most won't, its faster to use err.Error() than always use fmt to print the error.
Only the caller knows the appropriate style to use for the current context in the log line.
Blue looks good with some terminal themes, but not with the default macos theme. Cyan generally looks fine on more themes I think.
Open
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR combines the other PRs:
error
attr values also implementfmt.Formatter
, then print the error using fmt.Printf("%+v"). Several error packages use this convention for printing stack traces. Formatting errors #7I've benchmarked this against the main branch. Even when the features are used, there are still zero allocations. The only feature which introduces some performance overhead is the multiline value handling, which increased processing by about 75ns on my system (from about 825ns to 900ns). The other features have no impact when not used.
As an example, given this:
Output looks like:
Here's another sample, from macos Terminal.app, Basic Theme:
Note:
More work could be considered on multiline values, like placing the entire value on its own lines. Still playing with different options.