Conversation
trotterdylan
left a comment
trotterdylan
left a comment
There was a problem hiding this comment.
This is great! Thanks for working on this PR. I have a few comments.
runtime/builtin_types.go
Outdated
| } | ||
|
|
||
| func builtinFilter(f *Frame, args Args, _ KWArgs) (*Object, *BaseException) { | ||
| argc := len(args) |
There was a problem hiding this comment.
Can you instead use:
if raised := checkMethodArgs(f, "filter", args, ObjectType, ObjectType); raised != nil {
return nil, raised
}
runtime/builtin_types.go
Outdated
| if fn == None { | ||
| if ret, raised := IsTrue(f, item); raised != nil { | ||
| return nil, raised | ||
| } else if ret { |
There was a problem hiding this comment.
Remove the else since the if clause above returns
runtime/builtin_types.go
Outdated
| } | ||
| if ret, raised := IsTrue(f, ret); raised != nil { | ||
| return nil, raised | ||
| } else if ret { |
| fn := args[0] | ||
| l := args[1] | ||
| switch { | ||
| case l.isInstance(TupleType): |
There was a problem hiding this comment.
I was puzzled about this special case, but then I realized that CPython is doing the same kind of special casing:
>>> class Empty(tuple):
... def __iter__(self):
... return self
... def next(self):
... raise StopIteration
...
>>> filter(None, Empty([1,2,3]))
(1, 2, 3)
IMO that's bad behavior, but since our goal is compatibility, I guess we need to special case as well. But I think a comment is warranted so that readers of the code understand why we're doing this.
runtime/builtin_types.go
Outdated
| } | ||
| if ret, raised := IsTrue(f, ret); raised != nil { | ||
| return nil, raised | ||
| } else if ret { |
runtime/builtin_types.go
Outdated
| return l, nil | ||
| } | ||
| var result bytes.Buffer | ||
| for _, item := range toStrUnsafe(l).Value() { |
There was a problem hiding this comment.
This will iterate over l assuming it's a UTF-8 encoded string. I think what you actually want is to convert this to a []byte and iterate over each byte and WriteByte() below instead of WriteRune().
runtime/builtin_types.go
Outdated
| if fn == None { | ||
| if ret, raised := IsTrue(f, item); raised != nil { | ||
| return raised | ||
| } else if ret { |
runtime/builtin_types.go
Outdated
| } | ||
| if ret, raised := IsTrue(f, ret); raised != nil { | ||
| return raised | ||
| } else if ret { |
runtime/builtin_types.go
Outdated
| default: | ||
| result := make([]*Object, 0) | ||
| raised := seqForEach(f, l, func(item *Object) (raised *BaseException) { | ||
| if fn == None { |
There was a problem hiding this comment.
Given you do this check a few times I wonder if you should do something like this at the top:
filterFunc := IsTrue
if fn != None {
filterFunc = func(f *Frame, o *Object) (bool, *BaseException) {
result, raised := fn.Call(f, Args{o}, nil)
if raised != nil {
return false, raised
}
return IsTrue(f, result)
}
}
And then use filterFunc throughout.
| {f: "filter", args: wrapArgs(BoolType, NewTuple2(NewInt(0).ToObject(), NewInt(1).ToObject())), want: NewTuple1(NewInt(1).ToObject()).ToObject()}, | ||
| {f: "filter", args: wrapArgs(IntType, "012"), want: NewStr("12").ToObject()}, | ||
| {f: "filter", args: wrapArgs(None, newTestList(1, 0, 3)), want: newTestList(1, 3).ToObject()}, | ||
| {f: "filter", args: wrapArgs(IntType, newTestList("1", "0", "3")), want: newTestList("1", "3").ToObject()}, |
There was a problem hiding this comment.
Can you add tests for subclasses of str and tuple that implement __iter__ to make sure we maintain compatibility with CPython?
There was a problem hiding this comment.
Hi can i add these tests in testing/ directory using Python?
There was a problem hiding this comment.
I'd like tests in the Go code because it's easy to measure coverage there. Additional tests in testing/ are fine though.
|
@trotterdylan Updated! |
2 participants
No description provided.