Single line pretty-printing options #20
Merged
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
veqryn
commented
Nov 2, 2023
| @@ -274,6 +274,7 @@ func (e *encoderState) UnwriteEmptyObjectMember(prevName *string) bool { | |||
| b = b[:len(b)-n] | |||
| b = jsonwire.TrimSuffixWhitespace(b) | |||
| b = jsonwire.TrimSuffixByte(b, ':') | |||
| b = jsonwire.TrimSuffixWhitespace(b) | |||
There was a problem hiding this comment.
i think this is needed, but am not really sure about this section of code or why it worked previously without it even on multiline/expanded json.
|
Thanks for the PR. I apologize for being a stickler for legal rules. Since the goal of this is intended to merge into the Go toolchain, we'll need every contribution to have also signed the Go CLA. Is there a merged commit in any of the Go repositories that you can point to that proves you've signed the CLA? Thanks! High-level thought: Modifying |
|
Of course. I have signed the Go CLA. Here is my most recent contribution, though small: |
|
I am also curious on how it may affect performance. Do you want me to run benchmarks, and do those benchmarks already exist? Or is this something you'd like to do to keep consistency with previous runs? |
|
Ran some benchmarks, comparing against master. Master: Single line pretty printing PR: Comparison between master and this pr: @dsnet What do you think? |
|
Thanks for running the benchmarks, but up to 10% slowdown is a fairly non-trivial performance detriment. Especially since the benchmarks are intended to exercise the performance Go reflection, not the syntactic processor. |
|
If you're okay with it, I can take a stab at implementing this. |
|
Ok. So here is what I plan to change, please let me know what you think:
if optimizeCommon && !xe.Flags.Get(jsonflags.Expand) && !xe.Tokens.Last.NeedObjectName() {
xe.Buf = append(xe.Tokens.MayAppendDelim(xe.Buf, '{', xe.Flags.Get(jsonflags.SpaceAfterColon), xe.Flags.Get(jsonflags.SpaceAfterComma)), "{}"...)I will either add |
|
I'm ok if you want to take a stab. I'd probably prefer to collaborate so I can learn from you (including if we want to throw all of this out and start over, with different flags, etc), but am ok with anything. |
|
Feel free to keep working on it. Your idea is similar to what I had in mind. Essentially, we want to change nothing about the old code path if You can probably do something like: package jsonflags
// AnyWhitespace reports whether the encoded output might have any whitespace.
const AnyWhitespace = SpaceAfterColon | SpaceAfterComma | Expandpackage json
if optimizeCommon && !xe.Flags.Get(jsonflags.AnyWhitespace) && !xe.Tokens.Last.NeedObjectName() {
...
}I believe this should work since We might want to rename |
|
We also probably want to have the semantic that use of |
|
Also for benchmarks, you probably want to run these instead: |
veqryn
force-pushed
the
veqryn/single-line-pretty
branch
from
4df96e6 to
ab209a7
Compare
|
Master: This PR: |
|
looks like most of the benchmarks are within a half-percent of each other, then there are a few that are 2% faster with the PR, and a few more than are 3-4% slower with the PR. Margin of error is probably 2-3% on my laptop for small runs like this. |
|
@dsnet The PR was updated with your suggestions. Please let me know what you think. |
|
Thanks for working on this, I'm a bit busy this week, so I might be able to get to this on the weekend. |
|
hey @dsnet , just wondering if we can get this merged in now :) |
|
rebased on master... |
|
hi @dsnet , I am now using JSON v2 in production, via my v2 json slog handler: https://github.com/veqryn/slog-json However, because go.mod replacement rules don't work well for libraries, I am forced to point slog-json at my fork of JSON v2 in order to get the single-line pretty-printing. When we left off with this PR it sounded like you were ready to merge it after a final re-review. Is there anything I can do to get this moving again, please? |
|
Generally, LGTM. There are some cleanups and minor behavior changes, but it'll probably easier to just change that after submission than have a few more rounds of back-and-forth review. |
|
Sounds great |
|
Thank you for the contribution. I apologize for the massive delay. |
|
You are very welcome |
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.
Discussion:
In the same vein as the
WithIndentandWithIndentPrefixformatting options, I would really like the option to make json pretty but on a single line.I end up reading, or at least skimming, through a lot of new-line delimited json, and this would make my life better 100%.
Consider reading the following:
The first two are much easier to see the key-value pairs and the elements than the third one, which is the only option right now.
golang/go#63397 (comment)
PR Details:
This PR adds two new boolean flag options:
SpaceAfterColonandSpaceAfterCommaI do like those to be separate options, as I prefer to have the space after comma but not after colon, while lots of other people like space after both.
I'm not married to the naming or the implementation, and am open to any suggestions.
Please let me know if I need to add any additional tests beyond the two I added.