From b3f7cd73e6f91a7ad927e0b468f4b099d64a24a0 Mon Sep 17 00:00:00 2001
From: Donal Simmie <donal.simmie@gmail.com>
Date: Sun, 6 May 2018 15:07:56 +0100
Subject: [PATCH] Fixed review comments from #1930

---
 python/pyarrow/builder.pxi           | 20 +++----
 python/pyarrow/tests/test_builder.py | 89 +++++-----------------------
 2 files changed, 26 insertions(+), 83 deletions(-)

diff --git a/python/pyarrow/builder.pxi b/python/pyarrow/builder.pxi
index 8c9ba973b2712..5fa3054166785 100644
--- a/python/pyarrow/builder.pxi
+++ b/python/pyarrow/builder.pxi
@@ -38,16 +38,16 @@ cdef class StringBuilder:
         for value in values:
             self.append(value)
 
-    def length(self):
-        return self.builder.get().length()
+    def finish(self):
+        cdef shared_ptr[CArray] out
+        with nogil:
+            self.builder.get().Finish(&out)
+        return pyarrow_wrap_array(out)
 
-    def null_count(self):
-        return self.builder.get().null_count()
+    property null_count:
 
-    def finish(self):
-        return pyarrow_wrap_array(self._finish())
+        def __get__(self):
+            return self.builder.get().null_count()
 
-    cdef shared_ptr[CArray] _finish(self) nogil:
-        cdef shared_ptr[CArray] out
-        self.builder.get().Finish(&out)
-        return out
+    def __len__(self):
+        return self.builder.get().length()
diff --git a/python/pyarrow/tests/test_builder.py b/python/pyarrow/tests/test_builder.py
index 89398280a14c9..a9ad9bfea416d 100644
--- a/python/pyarrow/tests/test_builder.py
+++ b/python/pyarrow/tests/test_builder.py
@@ -15,99 +15,42 @@
 # specific language governing permissions and limitations
 # under the License.
 
-import pytest
-import pyarrow as pa
-from pyarrow.lib import StringBuilder
 import numpy as np
-import pandas as pd
-
 
-@pytest.fixture
-def sbuilder():
-    return StringBuilder()
+import pyarrow as pa
+from pyarrow.lib import StringBuilder
 
 
-def test_string_builder_append_string(sbuilder):
+def test_string_builder_append_string():
+    sbuilder = StringBuilder()
     sbuilder.append(b'jsaafakjh')
-    assert (sbuilder.length() == 1)
     sbuilder.append('jsaafakjh')
-    assert (sbuilder.length() == 2)
-    arr = sbuilder.finish()
-    assert (sbuilder.length() == 0)
-    assert (isinstance(arr, pa.Array))
-    assert (arr.null_count == 0)
-    assert (arr.type == 'str')
-    assert(len(arr.to_pylist()) == 2)
-
-
-def test_string_builder_append_none(sbuilder):
-    sbuilder.append(None)
-    assert (sbuilder.null_count() == 1)
-    for i in range(10):
-        sbuilder.append(None)
-    assert (sbuilder.null_count() == 11)
-    arr = sbuilder.finish()
-    assert (arr.null_count == 11)
-
-
-def test_string_builder_append_nan(sbuilder):
-    sbuilder.append(np.nan)
-    sbuilder.append(np.nan)
-    assert (sbuilder.null_count() == 2)
-    arr = sbuilder.finish()
-    assert (arr.null_count == 2)
-
-
-def test_string_builder_append_both(sbuilder):
     sbuilder.append(np.nan)
     sbuilder.append(None)
-    sbuilder.append("Some text")
-    sbuilder.append("Some text")
-    sbuilder.append(np.nan)
-    sbuilder.append(None)
-    assert (sbuilder.null_count() == 4)
+    assert (len(sbuilder) == 4)
     arr = sbuilder.finish()
-    assert (arr.null_count == 4)
-    expected = [None, None, "Some text", "Some text", None, None]
-    assert (arr.to_pylist() == expected)
+    assert (len(sbuilder) == 0)
+    assert (sbuilder.null_count == 2)
+    assert (isinstance(arr, pa.Array))
+    assert (arr.null_count == 2)
+    assert (arr.type == 'str')
+    assert(len(arr.to_pylist()) == 4)
 
 
-def test_string_builder_append_pylist(sbuilder):
+def test_string_builder_append_pylist():
+    sbuilder = StringBuilder()
     sbuilder.append_values([np.nan, None, "text", None, "other text"])
-    assert (sbuilder.null_count() == 3)
+    assert (sbuilder.null_count == 3)
     arr = sbuilder.finish()
     assert (arr.null_count == 3)
     expected = [None, None, "text", None, "other text"]
     assert (arr.to_pylist() == expected)
 
 
-def test_string_builder_append_series(sbuilder):
-    sbuilder.append_values(
-        pd.Series([np.nan, None, "text", None, "other text"])
-    )
-    assert (sbuilder.null_count() == 3)
-    arr = sbuilder.finish()
-    assert (arr.null_count == 3)
-    expected = [None, None, "text", None, "other text"]
-    assert (arr.to_pylist() == expected)
-
-
-def test_string_builder_append_numpy(sbuilder):
-    sbuilder.append_values(
-        np.asarray([np.nan, None, "text", None, "other text"])
-    )
-    assert (sbuilder.null_count() == 3)
-    arr = sbuilder.finish()
-    assert (arr.null_count == 3)
-    expected = [None, None, "text", None, "other text"]
-    assert (arr.to_pylist() == expected)
-
-
-def test_string_builder_append_after_finish(sbuilder):
+def test_string_builder_append_after_finish():
+    sbuilder = StringBuilder()
     sbuilder.append_values([np.nan, None, "text", None, "other text"])
-    assert (sbuilder.null_count() == 3)
     arr = sbuilder.finish()
     sbuilder.append("No effect")
-    assert (arr.null_count == 3)
     expected = [None, None, "text", None, "other text"]
     assert (arr.to_pylist() == expected)