From ecce1f6be9fb2a177e1c61577d5f1fd16e36cc80 Mon Sep 17 00:00:00 2001
From: ruby0x1 <ruby0x1@pm.me>
Date: Sat, 3 Apr 2021 19:55:42 -0700
Subject: [PATCH] List; add remove(value)

Having to encode this behaviour at every call site is tedious. It makes a lot of sense to just have the method available on list itself.
---
 doc/site/lists.markdown             | 18 +++++++++++++++---
 doc/site/modules/core/list.markdown | 20 ++++++++++++++++++++
 src/vm/wren_core.c                  |  8 ++++++++
 test/core/list/remove.wren          | 18 ++++++++++++++++++
 4 files changed, 61 insertions(+), 3 deletions(-)
 create mode 100644 test/core/list/remove.wren

diff --git a/doc/site/lists.markdown b/doc/site/lists.markdown
index 7ed6bacd..1f471c4f 100644
--- a/doc/site/lists.markdown
+++ b/doc/site/lists.markdown
@@ -113,21 +113,33 @@ System.print(combined)  //> [a, b, c, d, e, f]
 ## Removing elements
 
 The opposite of `insert` is `removeAt`. It removes a single element from a
-given position in the list. All following items are shifted up to fill in the
-gap:
+given position in the list. 
+
+To remove a specific _value_ instead, use `remove`. The first value that 
+matches using regular equality will be removed.
+
+In both cases, all following items are shifted up to fill in the gap.
 
 <pre class="snippet">
 var letters = ["a", "b", "c", "d"]
 letters.removeAt(1)
 System.print(letters) //> [a, c, d]
+letters.remove("a")
+System.print(letters) //> [c, d]
 </pre>
 
-The `removeAt` method returns the removed item:
+Both the `remove` and `removeAt` method return the removed item:
 
 <pre class="snippet">
 System.print(letters.removeAt(1)) //> c
 </pre>
 
+If `remove` couldn't find the value in the list, it returns null:
+
+<pre class="snippet">
+System.print(letters.remove("not found")) //> null
+</pre>
+
 If you want to remove everything from the list, you can clear it:
 
 <pre class="snippet">
diff --git a/doc/site/modules/core/list.markdown b/doc/site/modules/core/list.markdown
index 879de2ed..b02fddfd 100644
--- a/doc/site/modules/core/list.markdown
+++ b/doc/site/modules/core/list.markdown
@@ -96,6 +96,26 @@ list.
 
 [iterator protocol]: ../../control-flow.html#the-iterator-protocol
 
+### **remove**(value)
+
+Removes the first value found in the list that matches the given `value`, 
+using regular equality to compare them. All trailing elements
+are shifted up to fill in where the removed element was.
+
+<pre class="snippet">
+var list = ["a", "b", "c", "d"]
+list.remove("b")
+System.print(list) //> [a, c, d]
+</pre>
+
+Returns the removed value, if found.
+If the value is not found in the list, returns null.
+
+<pre class="snippet">
+System.print(["a", "b", "c"].remove("b")) //> b
+System.print(["a", "b", "c"].remove("not found")) //> null
+</pre>
+
 ### **removeAt**(index)
 
 Removes the element at `index`. If `index` is negative, it counts backwards
diff --git a/src/vm/wren_core.c b/src/vm/wren_core.c
index 1abfff68..ef72ccd0 100644
--- a/src/vm/wren_core.c
+++ b/src/vm/wren_core.c
@@ -394,6 +394,13 @@ DEF_PRIMITIVE(list_removeAt)
   RETURN_VAL(wrenListRemoveAt(vm, list, index));
 }
 
+DEF_PRIMITIVE(list_removeValue) {
+  ObjList* list = AS_LIST(args[0]);
+  int index = wrenListIndexOf(vm, list, args[1]);
+  if(index == -1) RETURN_NULL;
+  RETURN_VAL(wrenListRemoveAt(vm, list, index));
+}
+
 DEF_PRIMITIVE(list_indexOf)
 {
   ObjList* list = AS_LIST(args[0]);
@@ -1411,6 +1418,7 @@ void wrenInitializeCore(WrenVM* vm)
   PRIMITIVE(vm->listClass, "iterate(_)", list_iterate);
   PRIMITIVE(vm->listClass, "iteratorValue(_)", list_iteratorValue);
   PRIMITIVE(vm->listClass, "removeAt(_)", list_removeAt);
+  PRIMITIVE(vm->listClass, "remove(_)", list_removeValue);
   PRIMITIVE(vm->listClass, "indexOf(_)", list_indexOf);
   PRIMITIVE(vm->listClass, "swap(_,_)", list_swap);
 
diff --git a/test/core/list/remove.wren b/test/core/list/remove.wren
new file mode 100644
index 00000000..6e6a5249
--- /dev/null
+++ b/test/core/list/remove.wren
@@ -0,0 +1,18 @@
+var a = [1, 2, 3]
+a.remove(2)
+System.print(a) // expect: [1, 3]
+
+var b = [1, 2, 3]
+b.remove(1)
+System.print(b) // expect: [2, 3]
+
+var c = [1, 2, 3]
+c.remove(3)
+System.print(c) // expect: [1, 2]
+
+// Return the removed value.
+System.print([3, 4, 5].remove(4)) // expect: 4
+System.print([3, 4, 5].remove(5)) // expect: 5
+
+// Return null when not found
+System.print([1, 2, 3].remove(8)) // expect: null