Skip to content
This repository has been archived by the owner on Mar 23, 2023. It is now read-only.

Add builtin sorted #100

Merged
merged 6 commits into from
Jan 13, 2017
Merged

Add builtin sorted #100

merged 6 commits into from
Jan 13, 2017

Conversation

S-YOU
Copy link
Contributor

@S-YOU S-YOU commented Jan 12, 2017

Add builtin sorted, without cmp, key, and reverse options.

@S-YOU S-YOU changed the title Add builtin sorted Add builtin sorted and reversed Jan 12, 2017
Copy link
Contributor

@meadori meadori left a comment

Choose a reason for hiding this comment

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

The sorted implementation seems reasonable to me. Some tests in testing/builtin_test.py to exercise sorted from the Python side would be nice too.

Copy link
Contributor

@meadori meadori left a comment

Choose a reason for hiding this comment

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

I don't think this is the correct implementation. reversed is not actually a builtin function. It is a constructor for a builtin type. The builtin reversed type must also support the __reverse__ method: https://docs.python.org/2.7/reference/datamodel.html#object.__reversed__

@S-YOU
Copy link
Contributor Author

S-YOU commented Jan 12, 2017

Thanks @meadori, I didn't realize there is tests for python side.

Looks like I shouldn't include reversed in this PR. I didnt know about there is __reverse__ slot.

@meadori
Copy link
Contributor

meadori commented Jan 12, 2017

Yeah, I would break the reversed one out into a separate PR. It will probably be more involved.

@S-YOU S-YOU changed the title Add builtin sorted and reversed Add builtin sorted Jan 12, 2017
@S-YOU
Copy link
Contributor Author

S-YOU commented Jan 12, 2017

@meadori, Thank you, removed reversed

if raised := checkFunctionArgs(f, "sorted", args, ObjectType); raised != nil {
return nil, raised
}
result, raised := listNew(f, ListType, Args{args[0]}, KWArgs{})
Copy link
Contributor

Choose a reason for hiding this comment

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

ListType.Call(f, Args{args[0]}, nil)

@@ -541,6 +541,18 @@ func builtinRepr(f *Frame, args Args, kwargs KWArgs) (*Object, *BaseException) {
return s.ToObject(), nil
}

func builtinSorted(f *Frame, args Args, _ KWArgs) (*Object, *BaseException) {
if raised := checkFunctionArgs(f, "sorted", args, ObjectType); raised != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

sorted() is a little more complicated than this:

sorted(iterable, cmp=None, key=None, reverse=False) --> new sorted list

A TODO is fine. Then that functionality could be added to list.sort() and then passed when you call list.sort() below.

Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that as well, but didn't explicitly mention it b/c @S-YOU put Add builtin sorted, without cmp, key, and reverse options. in the change request message.

@@ -228,6 +228,13 @@ func TestBuiltinFuncs(t *testing.T) {
{f: "repr", args: wrapArgs(NewUnicode("abc")), want: NewStr("u'abc'").ToObject()},
{f: "repr", args: wrapArgs(newTestTuple("foo", "bar")), want: NewStr("('foo', 'bar')").ToObject()},
{f: "repr", args: wrapArgs("a", "b", "c"), wantExc: mustCreateException(TypeErrorType, "'repr' requires 1 arguments")},
{f: "sorted", args: wrapArgs(NewList()), want: NewList().ToObject()},
{f: "sorted", args: wrapArgs(newTestList("foo", "bar")), want: newTestList("bar", "foo").ToObject()},
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe some test cases that are other iterable types like dict and tuple.

@S-YOU
Copy link
Contributor Author

S-YOU commented Jan 13, 2017

Updated following

  • TODO about (cmp=None, key=None, reverse=False)
  • Use ListType.Call instead of listNew
  • Added tests for Tuple and Dict
  • Added some python side testing

Please take a look, @meadori, @trotterdylan

@@ -541,6 +541,19 @@ func builtinRepr(f *Frame, args Args, kwargs KWArgs) (*Object, *BaseException) {
return s.ToObject(), nil
}

func builtinSorted(f *Frame, args Args, _ KWArgs) (*Object, *BaseException) {
//TODO: Support (cmp=None, key=None, reverse=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

Space after // in comments:

// TODO: ...

Copy link
Contributor

@trotterdylan trotterdylan left a comment

Choose a reason for hiding this comment

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

This is great! one last nit.

@S-YOU
Copy link
Contributor Author

S-YOU commented Jan 13, 2017

@trotterdylan, added space after comment //, thank you very much.

@trotterdylan trotterdylan merged commit 384954e into google:master Jan 13, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants