Implement the 'reverse' option for sorted()

[MirkoDziadzka]

Add the 'reverse' option to 'sorted()'

I'm not sure if this violates the 'stable sort definition in Python.
On the other hand, this is probably only relevant when key or cmp gets implemented.

[trotterdylan]

Thanks for working on this! I have a couple high level comments.

[trotterdylan]

Note that reverse can be passed positionally as well:

>>> sorted([1,2,3], None, None, True)
[3, 2, 1]

This is a little tricky to get right with this Args, KWArgs calling convention. You need to check that the argument is supplied in args, but not kwargs, or vice versa. CPython with the PyArg_Parse*() functions. We don't yet have such a utility.

One option that does exist now is to create a Code object for the builtin which will do the basic argument validation for you. It's a little more involved than newBuiltinFunction though:

var builtinSortedArgs = []FunctionArg{{"iterable", nil}, {"cmp", None}, {"key", None}, {"reverse", False.ToObject()}}
func builtinSorted(f *Frame, args []*Object) {
  // args is guaranteed to be a slice of length 4 with no nil elements
}
...
"sorted": NewFunction(NewCode("sorted", "<builtin>", builtinSortedArgs, 0, builtinSorted), nil).ToObject(),

This could probably be simplified with some kind of newBuiltinCodeFunction() helper or something.

So there's currently no great out of the box options for doing what you need to do, but it seems like we're going to need to establish a pattern here before long.

[MirkoDziadzka]

Maybe we should not implement this in sorted(), but in listSort and just change the sorted implementation here to call

-func builtinSorted(f *Frame, args Args, _ KWArgs) (*Object, *BaseException) {
+func builtinSorted(f *Frame, args Args, kwargs KWArgs) (*Object, *BaseException) {
        // TODO: Support (cmp=None, key=None, reverse=False)
        if raised := checkFunctionArgs(f, "sorted", args, ObjectType); raised != nil {
                return nil, raised
@@ -579,7 +579,13 @@ func builtinSorted(f *Frame, args Args, _ KWArgs) (*Object, *BaseException) {
        if raised != nil {
                return nil, raised
        }
-       toListUnsafe(result).Sort(f)
+       // Creating newArgs by replacing the first element with teh new list.
+       newArgs := args.makeCopy()
+       newArgs[0] = result
+       _, raised = listSort(f, newArgs, kwargs)
+       if raised != nil {
+               return nil, raised
+       }
        return result, nil
 }

[trotterdylan]

SGTM

[trotterdylan]

As you mention, stability of the results is a concern here. If we supported the cmp param, we could negate the result of that to achieve reversal. This would mean that listSorter would need to support cmp. Then sorted(... reverse=True) could wrap that cmp and negate the result (or provide a stock reverse cmp when cmp=None).

So maybe it makes sense to implement cmp first?

[MirkoDziadzka]

Ack. But I would prefer to implement this in listSort and changing sorted() to

-func builtinSorted(f *Frame, args Args, _ KWArgs) (*Object, *BaseException) {
+func builtinSorted(f *Frame, args Args, kwargs KWArgs) (*Object, *BaseException) {
        // TODO: Support (cmp=None, key=None, reverse=False)
        if raised := checkFunctionArgs(f, "sorted", args, ObjectType); raised != nil {
                return nil, raised
@@ -579,7 +579,13 @@ func builtinSorted(f *Frame, args Args, _ KWArgs) (*Object, *BaseException) {
        if raised != nil {
                return nil, raised
        }
-       toListUnsafe(result).Sort(f)
+       // Creating newArgs by replacing the first element with teh new list.
+       newArgs := args.makeCopy()
+       newArgs[0] = result
+       _, raised = listSort(f, newArgs, kwargs)
+       if raised != nil {
+               return nil, raised
+       }
        return result, nil
 }

[trotterdylan]

That all sounds good. Let me know when I should review again.