Skip to content

Add escaping text transformer#339

Closed
SamWhited wants to merge 1 commit intorivo:masterfrom
SamWhited:escape_transformer
Closed

Add escaping text transformer#339
SamWhited wants to merge 1 commit intorivo:masterfrom
SamWhited:escape_transformer

Conversation

@SamWhited
Copy link
Copy Markdown
Contributor

@SamWhited SamWhited commented Aug 25, 2019

In an application I am populating a text view that uses colors from a file and do not want to buffer the entire file in memory multiple times or wait for the regexp to run over the entire file (which needs to be escaped to ensure that the user cannot inject colors or regions into the view).
Since text views are writers and the file is being copied in, I needed a wrapper that could wrap the writer and perform the escaping on the entire file as it was copied. A very nice API for dealing with text transformations like this already exists in the golang.org/x/text/transform package, including a way to apply the transformer to a writer. I originally wrote this for my package, but thought it might be useful for others as well and decided to submit it here.

This patch adds a transformer that can be used to escape text and re-implements the Escape function in terms of this transformer (resulting in a roughly 2x speedup on simple benchmarks). To keep the code simple and avoid having to redo all the standard transformer logic, the github.com/mpvl/textutil package was used. This means there is the potential to remove the dependency on textutil in the future and possibly further increase the speed of the escaper at the cost of lots more boilerplate in this package.

I did not include tests in this PR because on a previous change you told me that you didn't want them, however, the tests that I used to verify that the changes matched the old behavior are below. I would by very happy to include them in the PR if you change your mind about testing.

Thanks for your consideration!

Benchmark results
$ go test -v -bench . -benchmem
=== RUN   TestEscape
=== RUN   TestEscape/0
=== RUN   TestEscape/0/Legacy
=== RUN   TestEscape/0/Transform
=== RUN   TestEscape/1
=== RUN   TestEscape/1/Legacy
=== RUN   TestEscape/1/Transform
=== RUN   TestEscape/2
=== RUN   TestEscape/2/Legacy
=== RUN   TestEscape/2/Transform
=== RUN   TestEscape/3
=== RUN   TestEscape/3/Legacy
=== RUN   TestEscape/3/Transform
=== RUN   TestEscape/4
=== RUN   TestEscape/4/Legacy
=== RUN   TestEscape/4/Transform
--- PASS: TestEscape (0.00s)
    --- PASS: TestEscape/0 (0.00s)
        --- PASS: TestEscape/0/Legacy (0.00s)
        --- PASS: TestEscape/0/Transform (0.00s)
    --- PASS: TestEscape/1 (0.00s)
        --- PASS: TestEscape/1/Legacy (0.00s)
        --- PASS: TestEscape/1/Transform (0.00s)
    --- PASS: TestEscape/2 (0.00s)
        --- PASS: TestEscape/2/Legacy (0.00s)
        --- PASS: TestEscape/2/Transform (0.00s)
    --- PASS: TestEscape/3 (0.00s)
        --- PASS: TestEscape/3/Legacy (0.00s)
        --- PASS: TestEscape/3/Transform (0.00s)
    --- PASS: TestEscape/4 (0.00s)
        --- PASS: TestEscape/4/Legacy (0.00s)
        --- PASS: TestEscape/4/Transform (0.00s)
goos: linux
goarch: amd64
pkg: github.com/rivo/tview
BenchmarkLegacy-8         397126              2676 ns/op             379 B/op         12 allocs/op
BenchmarkEscape-8         825540              1352 ns/op             434 B/op          5 allocs/op
BenchmarkTransform-8      887028              1240 ns/op             304 B/op          2 allocs/op
PASS
ok      github.com/rivo/tview   3.345s
escape_test.go
package tview_test

import (
	"regexp"
	"strconv"
	"testing"

	"github.com/rivo/tview"
	"golang.org/x/text/transform"
)

var nonEscapePattern = regexp.MustCompile(`(\[[a-zA-Z0-9_,;: \-\."#]+\[*)\]`)

func legacyEscape(text string) string {
	return nonEscapePattern.ReplaceAllString(text, "$1[]")
}

var escapeTests = [...]struct {
	in, out string
}{
	0: {},
	1: {in: `["abc"][""][][red]`, out: `["abc"[][""[][][red[]`},
	2: {in: `[""[[[]`, out: `[""[[[[]`},
	3: {in: `["a[bc"]`, out: `["a[bc"[]`},
	4: {in: `["a]bc"]`, out: `["a[]bc"]`},
}

func TestEscape(t *testing.T) {
	for i, tc := range escapeTests {
		t.Run(strconv.Itoa(i), func(t *testing.T) {
			t.Run("Legacy", func(t *testing.T) {
				out := legacyEscape(tc.in)
				if out != tc.out {
					t.Errorf("want=`%s`, got=`%s`", tc.out, out)
				}
			})
			t.Run("Transform", func(t *testing.T) {
				et := tview.EscapeTransformer()
				out, _, err := transform.String(et, tc.in)
				if err != nil {
					t.Errorf("Unexpected error: %v", err)
				}
				if out != tc.out {
					t.Errorf("want=`%s`, got=`%s`", tc.out, out)
				}
			})
		})
	}
}

const benchEscape = `["abc"][""][][red][""[[[]["a[bc"]["a]bc"]`

func BenchmarkLegacy(b *testing.B) {
	for i := 0; i < b.N; i++ {
		_ = legacyEscape(benchEscape)
	}
}

func BenchmarkEscape(b *testing.B) {
	for i := 0; i < b.N; i++ {
		_ = tview.Escape(benchEscape)
	}
}

func BenchmarkTransform(b *testing.B) {
	t := tview.EscapeTransformer()
	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		_, _, _ = transform.String(t, benchEscape)
	}
}

SamWhited added a commit to mellium/communique-tui that referenced this pull request Aug 25, 2019
This package will be removed if it is merged into tview.

See rivo/tview#339.
@SamWhited
Copy link
Copy Markdown
Contributor Author

@rivo ping; is this something you'd be interested in at all? Thanks.

@SamWhited
Copy link
Copy Markdown
Contributor Author

Closing this to get it off my dashboard since it seems that it's not wanted. We can always revisit if it's desired later.

@SamWhited SamWhited closed this May 26, 2021
@rivo
Copy link
Copy Markdown
Owner

rivo commented May 27, 2021

Got this in my notifications. Apologies for not responding here. But as mentioned elsewhere, I'm already behind on my issues and PRs are a lot more work to go through than issues so they will always end up on the backburner.

It doesn't mean it's not wanted. There's just no time for it.

@SamWhited
Copy link
Copy Markdown
Contributor Author

Not to worry, thanks for the response.

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