-
Notifications
You must be signed in to change notification settings - Fork 9
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
RFC - format
implementation
#32
Draft
reubent
wants to merge
1
commit into
luthersystems:main
Choose a base branch
from
reubent:some_tests_and_a_better_format_implementation
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.
Draft
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,185 @@ | ||
package lisp | ||
|
||
import ( | ||
"fmt" | ||
"regexp" | ||
"strconv" | ||
"strings" | ||
"testing" | ||
|
||
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
func TestFormatStringSimple(t *testing.T) { | ||
env := NewEnv(nil) | ||
format := String("Swap a symbol here {}") | ||
arg1 := String("yeah") | ||
args := &LVal{Cells: []*LVal{format, arg1}} | ||
formatted := builtinFormatString(env, args) | ||
assert.Equal(t, "Swap a symbol here yeah", formatted.Str) | ||
} | ||
|
||
func TestFormatStringComplex(t *testing.T) { | ||
env := NewEnv(nil) | ||
format := String("Swap a symbol here ") | ||
built := format.Str | ||
args := []*LVal{format} | ||
for x := 0; x < 100; x++ { | ||
args[0] = String(fmt.Sprintf("%s {}, blah blah,", args[0].Str)) | ||
built = fmt.Sprintf("%s %d, blah blah,", built, x) | ||
args = append(args, String(strconv.Itoa(x))) | ||
formatted := builtinFormatString(env, &LVal{Cells: args}) | ||
assert.Equal(t, built, formatted.Str) | ||
} | ||
} | ||
|
||
func TestFormatStringUnmatched(t *testing.T) { | ||
env := NewEnv(nil) | ||
format := String("Swap a symbol here }") | ||
arg1 := String("yeah") | ||
args := &LVal{Cells: []*LVal{format, arg1}} | ||
formatted := builtinFormatString(env, args) | ||
assert.Equal(t, LError, formatted.Type) | ||
} | ||
|
||
func TestTokenizeFormatString(t *testing.T) { | ||
x := tokenizeFormatString("This is a formatted {} string yeah {}.") | ||
assert.Equal(t, []formatToken{ | ||
{typ: formatText, text: "This is a formatted "}, | ||
{typ: formatOpen, text: "{"}, | ||
{typ: formatClose, text: "}"}, | ||
{typ: formatText, text: " string yeah "}, | ||
{typ: formatOpen, text: "{"}, | ||
{typ: formatClose, text: "}"}, | ||
{typ: formatText, text: "."}, | ||
}, x) | ||
} | ||
|
||
var formatTokenRegexp = regexp.MustCompile(`\{(\d+)\}`) | ||
|
||
// var formatBadTokenPattern = regexp.MustCompile(`(?:\{[^\d\}]|[^\{\d]\})`) // This is slooooow... | ||
// var formatBadTokenPattern = regexp.MustCompile(`\{[^\d\}]`) // This is also slow enough that I'd ditch it | ||
|
||
func quickformatBuiltin(env *LEnv, args *LVal) *LVal { | ||
format := args.Cells[0] | ||
//if formatBadTokenPattern.MatchString(format.Str) { | ||
//return env.Errorf("unmatched token") | ||
//} | ||
replacements := make([]interface{}, len(args.Cells)-1) | ||
for k, v := range args.Cells[1:] { | ||
if v.Type == LString && !v.Quoted { | ||
replacements[k] = v.Str | ||
} else { | ||
replacements[k] = v.String() | ||
} | ||
} | ||
if format.Type != LString { | ||
return env.Errorf("first argument is not a string") | ||
} | ||
pattern := formatTokenRegexp.ReplaceAllString(strings.ReplaceAll(strings.ReplaceAll(format.Str, "%", "%%"), "{}", "%s"), "%[$1]s") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My brain hurts a bit trying to parse this. This also doesn't catch syntax errors at all (e.g. unclosed curly brace). |
||
return String(fmt.Sprintf(pattern, replacements...)) | ||
} | ||
|
||
func TestQuickFormatStringSimple(t *testing.T) { | ||
env := NewEnv(nil) | ||
format := String("Swap a symbol here {}") | ||
arg1 := String("yeah") | ||
args := &LVal{Cells: []*LVal{format, arg1}} | ||
formatted := quickformatBuiltin(env, args) | ||
assert.Equal(t, "Swap a symbol here yeah", formatted.Str) | ||
} | ||
|
||
func TestQuickFormatStringComplex(t *testing.T) { | ||
env := NewEnv(nil) | ||
format := String("Swap a symbol here ") | ||
built := format.Str | ||
args := []*LVal{format} | ||
for x := 0; x < 100; x++ { | ||
args[0] = String(fmt.Sprintf("%s {}, blah blah,", args[0].Str)) | ||
built = fmt.Sprintf("%s %d, blah blah,", built, x) | ||
args = append(args, String(strconv.Itoa(x))) | ||
formatted := quickformatBuiltin(env, &LVal{Cells: args}) | ||
assert.Equal(t, built, formatted.Str) | ||
} | ||
} | ||
|
||
func TestQuickFormatStringUnmatched(t *testing.T) { | ||
env := NewEnv(nil) | ||
format := String("Swap a symbol here }") | ||
arg1 := String("yeah") | ||
args := &LVal{Cells: []*LVal{format, arg1}} | ||
formatted := quickformatBuiltin(env, args) | ||
assert.Equal(t, LError, formatted.Type) | ||
} | ||
|
||
func TestQuickFormatStringPositional(t *testing.T) { | ||
env := NewEnv(nil) | ||
format := String("Swap a symbol here {2} and here {1}") | ||
arg1 := String("yeah") | ||
arg2 := String("oh") | ||
args := &LVal{Cells: []*LVal{format, arg1, arg2}} | ||
formatted := quickformatBuiltin(env, args) | ||
assert.Equal(t, "Swap a symbol here oh and here yeah", formatted.Str) | ||
} | ||
|
||
func BenchmarkQuickFormatString(t *testing.B) { | ||
env := NewEnv(nil) | ||
format := String("Swap a symbol here {} and {} and {}") | ||
for x := 0; x < t.N; x++ { | ||
args := []*LVal{format, String("A"), String("B"), String("C")} | ||
formatted := quickformatBuiltin(env, &LVal{Cells: args}) | ||
assert.Equal(t, "Swap a symbol here A and B and C", formatted.Str) | ||
} | ||
} | ||
|
||
func BenchmarkQuickFormatStringWithPositionalToken(t *testing.B) { | ||
env := NewEnv(nil) | ||
format := String("Swap a symbol here {} and {} and {1}") | ||
for x := 0; x < t.N; x++ { | ||
args := []*LVal{format, String("A"), String("B")} | ||
formatted := quickformatBuiltin(env, &LVal{Cells: args}) | ||
assert.Equal(t, "Swap a symbol here A and B and A", formatted.Str) | ||
} | ||
} | ||
|
||
func BenchmarkQuickFormatStringWithATonOfTokens(t *testing.B) { | ||
env := NewEnv(nil) | ||
format := String("Swap a symbol here ") | ||
built := format.Str | ||
args := []*LVal{format} | ||
for x := 0; x < 100; x++ { | ||
args[0] = String(fmt.Sprintf("%s {}, blah blah,", args[0].Str)) | ||
built = fmt.Sprintf("%s %d, blah blah,", built, x) | ||
args = append(args, String(strconv.Itoa(x))) | ||
} | ||
for x := 0; x < t.N; x++ { | ||
formatted := quickformatBuiltin(env, &LVal{Cells: args}) | ||
assert.Equal(t, built, formatted.Str) | ||
} | ||
} | ||
|
||
func BenchmarkFormatString(t *testing.B) { | ||
env := NewEnv(nil) | ||
format := String("Swap a symbol here {} and {} and {}") | ||
for x := 0; x < t.N; x++ { | ||
args := []*LVal{format, String("A"), String("B"), String("C")} | ||
formatted := builtinFormatString(env, &LVal{Cells: args}) | ||
assert.Equal(t, "Swap a symbol here A and B and C", formatted.Str) | ||
} | ||
} | ||
|
||
func BenchmarkFormatStringWithATonOfTokens(t *testing.B) { | ||
env := NewEnv(nil) | ||
format := String("Swap a symbol here ") | ||
built := format.Str | ||
args := []*LVal{format} | ||
for x := 0; x < 100; x++ { | ||
args[0] = String(fmt.Sprintf("%s {}, blah blah,", args[0].Str)) | ||
built = fmt.Sprintf("%s %d, blah blah,", built, x) | ||
args = append(args, String(strconv.Itoa(x))) | ||
} | ||
for x := 0; x < t.N; x++ { | ||
formatted := builtinFormatString(env, &LVal{Cells: args}) | ||
assert.Equal(t, built, formatted.Str) | ||
} | ||
} |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there should be a test/error for unmatched tokens. The existing format-string does this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. Maybe this was trying to catch unclosed curly braces? What I meant was that format-string will report an error if you don't supply enough values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should do that - it's just
fmt.Sprintf
inside, so it'll handle it the same way that Go does with a warning in the output.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. fmt just outputs junk when there's a mismatch between argument and substitutions. But I don't think that is very good behavior for a language like lisp. I think a runtime error is preferable.
The annoying thing about the fmt package is that format argument mismatches can literally go unnoticed for onths or years. I think they chose to do that instead of forcing programmers to check errors for every formatting call. I've never really seen another argument for why formatting should work that way (though I guess I haven't looked hard if you had one).
In dynamic languages like python and ruby format argument mismatches cause runtime exceptions. Even if it's not completely preferable, I think it's more expected of a language like elps.