Skip to content

Implement json.encode_indent #615

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Oct 29, 2025
Merged

Implement json.encode_indent #615

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f2021cc
Implement json.encode_indent
tetromino Oct 23, 2025
de8947d
Remove encodeInternal - it only interferes with error formatting
tetromino Oct 23, 2025
d6a6ab5
Fix doc string
tetromino Oct 23, 2025
1913a3a
Save an allocation
tetromino Oct 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 30 additions & 3 deletions lib/json/json.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
//
// json = module(
// encode,
// encode_indent,
// decode,
// indent,
// )
Expand All @@ -54,6 +55,10 @@ import (
// (e.g. it implements both Iterable and HasFields), the first case takes precedence.
// Encoding any other value yields an error.
//
// def encode_indent(x, *, prefix="", indent="\t"):
//
// Equivalent to json.indent(json.encode(x), prefix=prefix, indent=indent).
//
// def decode(x[, default]):
//
// The decode function has one required positional parameter, a JSON string.
Expand All @@ -76,9 +81,10 @@ import (
var Module = &starlarkstruct.Module{
Name: "json",
Members: starlark.StringDict{
"encode": starlark.NewBuiltin("json.encode", encode),
"decode": starlark.NewBuiltin("json.decode", decode),
"indent": starlark.NewBuiltin("json.indent", indent),
"encode": starlark.NewBuiltin("json.encode", encode),
"encode_indent": starlark.NewBuiltin("json.encode_indent", encodeIndent),
"decode": starlark.NewBuiltin("json.decode", decode),
"indent": starlark.NewBuiltin("json.indent", indent),
},
}

Expand Down Expand Up @@ -233,6 +239,27 @@ func encode(thread *starlark.Thread, b *starlark.Builtin, args starlark.Tuple, k
return starlark.String(buf.String()), nil
}

func encodeIndent(thread *starlark.Thread, b *starlark.Builtin, args starlark.Tuple, kwargs []starlark.Tuple) (starlark.Value, error) {
prefix, indent := "", "\t" // keyword-only
if err := starlark.UnpackArgs(b.Name(), nil, kwargs,
"prefix?", &prefix,
"indent?", &indent,
); err != nil {
return nil, err
}
// Rely on encode() to parse the positional parameter (since the signature matches); use our b so
// error messages are attributed to json.encode_indent.
str, err := encode(thread, b, args, nil)
if err != nil {
return nil, err
}
buf := new(bytes.Buffer)
Copy link
Collaborator

@adonovan adonovan Oct 24, 2025

Choose a reason for hiding this comment

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

strings.Builder

will save an allocation.

Copy link
Collaborator Author

@tetromino tetromino Oct 24, 2025

Choose a reason for hiding this comment

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

json.Encode is built around growing a bytes.Buffer; I think making a version that appends to a strings.Builder is a bit out of scope of this pr (although potentially useful).

But you are right that allocating buf via new is a waste - fixed.

Copy link
Collaborator

@adonovan adonovan Oct 24, 2025

Choose a reason for hiding this comment

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

What I meant was that bytes.Buffer is more efficient when you want the final result as a []byte slice (buf.Bytes()), and strings.Builder is more efficient when you want the result as a string (buf.String()). (The difference is one allocation of a copy in each case.)

Since here we want String, we should use strings.Builder; no other change is needed.

BTW, allocating buf via new(T) or var x T in fact makes no difference: all that matters is whether the address of the variable escapes, which is independent of how you create the variable (unlike C++, where the syntax determines heap vs. stack allocation).

Copy link
Collaborator Author

@tetromino tetromino Oct 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Currently starlark-go relies on json.Indent for indenting, which writes its output into a *bytes.Buffer: https://pkg.go.dev/encoding/json#Indent

Maybe I'm missing something obvious, but I don't see how it can be efficiently made to write into a strings.Builder.

BTW, allocating buf via new(T) or var x T in fact makes no difference

Thank you for correcting my wrong mental model of Go's allocator!

Copy link
Collaborator

@adonovan adonovan Oct 29, 2025

Choose a reason for hiding this comment

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

Oh, good point, the standard encoding/json package is indeed a constraint. This is fine.

if err := json.Indent(buf, []byte(str.(starlark.String)), prefix, indent); err != nil {
return nil, fmt.Errorf("%s: %v", b.Name(), err)
}
return starlark.String(buf.String()), nil
}

func pointer(i interface{}) unsafe.Pointer {
v := reflect.ValueOf(i)
switch v.Kind() {
Expand Down
16 changes: 15 additions & 1 deletion starlark/testdata/json.star
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
load("assert.star", "assert")
load("json.star", "json")

assert.eq(dir(json), ["decode", "encode", "indent"])
assert.eq(dir(json), ["decode", "encode", "encode_indent", "indent"])

# Some of these cases were inspired by github.com/nst/JSONTestSuite.

Expand Down Expand Up @@ -49,6 +49,20 @@ recursive_tuple = (1, 2, [])
recursive_tuple[2].append(recursive_tuple)
encode_error(recursive_tuple, 'json.encode: at tuple index 2: at list index 0: cycle in JSON structure')

## json.encode_indent

assert.eq(
json.encode_indent({"x": 1, "y": ["one", "two"]}, prefix="¶", indent="–––"),
json.indent(json.encode({"x": 1, "y": ["one", "two"]}), prefix="¶", indent="–––")
)
assert.eq(
json.encode_indent({"x": 1, "y": ["one", "two"]}),
json.indent(json.encode({"x": 1, "y": ["one", "two"]}), prefix="", indent="\t")
)

assert.fails(lambda: json.encode_indent('foo', 'bar'), 'json.encode_indent: got 2 arguments, want 1')
assert.fails(lambda: json.encode_indent(recursive_map), 'json.encode_indent: in dict key "r": cycle in JSON structure')

## json.decode

assert.eq(json.decode("null"), None)
Expand Down
Loading