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

feat: redirect console output #542

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 15 additions & 2 deletions console.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package otto

import (
"fmt"
"io"
"os"
"strings"
)
Expand All @@ -15,12 +16,24 @@ func formatForConsole(argumentList []Value) string {
}

func builtinConsoleLog(call FunctionCall) Value {
fmt.Fprintln(os.Stdout, formatForConsole(call.ArgumentList)) //nolint:errcheck // Nothing we can do if this fails.
var w io.Writer
if call.runtime.stdoutWriter != nil {
w = call.runtime.stdoutWriter
} else {
w = os.Stdout
}
fmt.Fprintln(w, formatForConsole(call.ArgumentList)) //nolint:errcheck // Nothing we can do if this fails.
Comment on lines +19 to +25
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion: with the use of a default we can simply this.

Suggested change
var w io.Writer
if call.runtime.stdoutWriter != nil {
w = call.runtime.stdoutWriter
} else {
w = os.Stdout
}
fmt.Fprintln(w, formatForConsole(call.ArgumentList)) //nolint:errcheck // Nothing we can do if this fails.
fmt.Fprintln(call.runtime.console, formatForConsole(call.ArgumentList)) //nolint:errcheck // Nothing we can do if this fails.

return Value{}
}

func builtinConsoleError(call FunctionCall) Value {
fmt.Fprintln(os.Stdout, formatForConsole(call.ArgumentList)) //nolint:errcheck // Nothing we can do if this fails.
var w io.Writer
if call.runtime.stdoutWriter != nil {
w = call.runtime.stdoutWriter
} else {
w = os.Stdout
}
fmt.Fprintln(w, formatForConsole(call.ArgumentList)) //nolint:errcheck // Nothing we can do if this fails.
Comment on lines +30 to +36
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion: simplify as above.

Suggested change
var w io.Writer
if call.runtime.stdoutWriter != nil {
w = call.runtime.stdoutWriter
} else {
w = os.Stdout
}
fmt.Fprintln(w, formatForConsole(call.ArgumentList)) //nolint:errcheck // Nothing we can do if this fails.
fmt.Fprintln(call.runtime.console, formatForConsole(call.ArgumentList)) //nolint:errcheck // Nothing we can do if this fails.

return Value{}
}

Expand Down
14 changes: 13 additions & 1 deletion otto.go
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,7 @@ package otto
import (
"encoding/json"
"errors"
"io"
"strings"

"github.com/robertkrimen/otto/file"
Expand All @@ -238,12 +239,15 @@ type Otto struct {
}

// New will allocate a new JavaScript runtime.
func New() *Otto {
func New(opts ...Option) *Otto {
o := &Otto{
runtime: newContext(),
}
o.runtime.otto = o
o.runtime.traceLimit = 10
for _, opt := range opts {
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion: lets set the default so we can simplify the use after.

Suggested change
for _, opt := range opts {
o.runtime.console = os.Stdout
for _, opt := range opts {

opt(o)
}
if err := o.Set("console", o.runtime.newConsole()); err != nil {
panic(err)
}
Expand Down Expand Up @@ -775,3 +779,11 @@ func (o Object) MarshalJSON() ([]byte, error) {
}
return json.Marshal(goValue)
}

type Option func(*Otto)
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion: lets document the type.


func WithStdoutWriter(w io.Writer) Option {
return func(o *Otto) {
o.runtime.stdoutWriter = w
Comment on lines +785 to +787
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion: lets document the type and change the naming so it easier for the user to understand the impact of using this option.

Suggested change
func WithStdoutWriter(w io.Writer) Option {
return func(o *Otto) {
o.runtime.stdoutWriter = w
// WithConsoleOutput sets the target for console output.
// Default: Stdout.
func WithConsoleOutput(w io.Writer) Option {
return func(o *Otto) {
o.runtime.console = w

}
}
29 changes: 29 additions & 0 deletions otto_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1803,6 +1803,35 @@ func TestOttoInterrupt(t *testing.T) {
}
}

func TestOtterStdoutWriter(t *testing.T) {
tests := []struct {
name string
script string
expect string
}{
{
name: "console.log",
script: "console.log('hello', 'world')",
expect: "hello world\n",
},
{
name: "console.err",
script: "console.error('error', 'world')",
expect: "error world\n",
},
}

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
var buf = &bytes.Buffer{}
vm := New(WithStdoutWriter(buf))
_, err := vm.Run(tc.script)
require.NoError(t, err)
require.Equal(t, tc.expect, buf.String())
})
}
}
Comment on lines +1806 to +1833
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion: can we use sub tests vs table driven as its more concise and allows developers to run specific tests directly from their editor e.g. VS Code.

Also the use of just a declared buffer vs instantiated is more idiomatic.

Not tested the below, so there might be typos, but you get the idea 😃

Suggested change
func TestOtterStdoutWriter(t *testing.T) {
tests := []struct {
name string
script string
expect string
}{
{
name: "console.log",
script: "console.log('hello', 'world')",
expect: "hello world\n",
},
{
name: "console.err",
script: "console.error('error', 'world')",
expect: "error world\n",
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
var buf = &bytes.Buffer{}
vm := New(WithStdoutWriter(buf))
_, err := vm.Run(tc.script)
require.NoError(t, err)
require.Equal(t, tc.expect, buf.String())
})
}
}
func TestOtto_WithConsoleOutput(t *testing.T) {
t.Run('console.log', func(t *testing.T) {
var buf bytes.Buffer
vm := New(WithConsoleOutput(&buf))
_, err := vm.Run("console.log('hello', 'world')")
require.NoError(t, err)
require.Equal(t, "hello world\n", buf.String())
})
t.Run('console.err', func(t *testing.T) {
var buf bytes.Buffer
vm := New(WithConsoleOutput(&buf))
_, err := vm.Run("console.error('world')")
require.NoError(t, err)
require.Equal(t, "error world\n", buf.String())
})
}


func BenchmarkNew(b *testing.B) {
for i := 0; i < b.N; i++ {
New()
Expand Down
2 changes: 2 additions & 0 deletions runtime.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"encoding/json"
"errors"
"fmt"
"io"
"math"
"path"
"reflect"
Expand Down Expand Up @@ -66,6 +67,7 @@ type runtime struct {
stackLimit int
traceLimit int
lck sync.Mutex
stdoutWriter io.Writer
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion: make it clear the use vs the value

Suggested change
stdoutWriter io.Writer
console io.Writer

}

func (rt *runtime) enterScope(scop *scope) {
Expand Down