Improve coverage; fix some things

Technologicat · Technologicat · commit 89099087da50 · 2020-08-26T04:48:51.000+03:00
- `mogrify` now supports `ll` correctly, `Nil` is no longer processed
  (technically, this is a breaking change, but `mogrify` wasn't very useful on
   `cons` structures before this change, so it's unlikely to affect anything)
- fix a search-and-misplace (no `wrap` in `_StrReprEqMixin`, it was just `w`)
- remove a silly `except` in `frozendict`. As input, we support any mappings
  that `dict` does; in case it's fed something inappropriate, let the user know.
- add missing test cases
diff --git a/unpythonic/collections.py b/unpythonic/collections.py
@@ -19,7 +19,7 @@
 from operator import lt, le, ge, gt
 import threading
 
-from .llist import cons
+from .llist import cons, Nil
 from .misc import getattrrec
 from .env import env
 from .dynassign import _Dyn
@@ -65,8 +65,7 @@ def mogrify(func, container):
 
         - The ``cons`` container from ``unpythonic.llist`` (including the
           ``llist`` linked lists). This is treated with the general tree
-          strategy, so nested linked lists will be flattened, and the
-          final ``nil`` is also processed.
+          strategy. Any ``nil`` is passed through as-is.
 
     Any value that does not match any of these is treated as an atom.
 
@@ -81,7 +80,7 @@ def doit(x):
             y = [doit(elt) for elt in x]
             if hasattr(x, "clear"):
                 x.clear()  # list has this, but not guaranteed by MutableSequence
-            else:
+            else:  # pragma: no cover, no realistic `MutableSequence` that is missing `clear`?
                 while x:
                     x.pop()
             x.extend(y)
@@ -95,7 +94,7 @@ def doit(x):
             x.clear()
             if hasattr(x, "update"):
                 x.update(y)  # set has this, but not guaranteed by MutableSet
-            else:
+            else:  # pragma: no cover, no realistic `MutableSet` that is missing `update`?
                 for elt in y:
                     x.add(elt)
             return x
@@ -110,6 +109,8 @@ def doit(x):
             x.set(doit(x.get()))
             return x
         # immutable containers
+        elif isinstance(x, Nil):  # for unpythonic.llist.ll() support
+            return x
         elif isinstance(x, cons):
             return cons(doit(x.car), doit(x.cdr))
         elif isinstance(x, Some):
@@ -184,7 +185,7 @@ def f(b):
     """
     def __init__(self, x=None):
         self.x = x
-    def __repr__(self):
+    def __repr__(self):  # pragma: no cover
         return "box({})".format(repr(self.x))
     def __contains__(self, x):
         return self.x == x
@@ -234,7 +235,7 @@ class ThreadLocalBox(box):
     def __init__(self, x=None):
         self.storage = threading.local()
         self._default = x
-    def __repr__(self):
+    def __repr__(self):  # pragma: no cover
         """**WARNING**: the repr shows only the content seen by the current thread."""
         return "ThreadLocalBox({})".format(repr(self.get()))
     def __contains__(self, x):
@@ -278,7 +279,7 @@ class Some:
     """
     def __init__(self, x=None):
         self.x = x
-    def __repr__(self):
+    def __repr__(self):  # pragma: no cover
         return "Some({})".format(repr(self.x))
     def __contains__(self, x):
         return self.x == x
@@ -450,14 +451,11 @@ def __init__(self, *ms, **bindings):
         super().__init__()
         self._data = {}
         for m in ms:
-            try:
-                self._data.update(m)
-            except TypeError:
-                pass
+            self._data.update(m)
         self._data.update(bindings)
 
     @wraps(dict.__repr__)
-    def __repr__(self):
+    def __repr__(self):  # pragma: no cover
         return "frozendict({})".format(self._data.__repr__())
 
     def __hash__(self):
@@ -534,30 +532,30 @@ class MutableSequenceView(SequenceView):
     """
     @abstractmethod
     def __setitem__(self, k, v):
-        pass
+        pass  # pragma: no cover
     @abstractmethod
     def reverse(self):
-        pass
+        pass  # pragma: no cover
 
 # -----------------------------------------------------------------------------
 
 class _StrReprEqMixin:
-    def _lowlevel_repr(self):
+    def _lowlevel_repr(self):  # pragma: no cover
         cls = type(getattrrec(self, "seq"))  # de-onionize
         ctor = tuple if hasattr(cls, "_make") else cls  # slice of namedtuple -> tuple
         return ctor(x for x in self)
-    def __str__(self):
+    def __str__(self):  # pragma: no cover
         return str(self._lowlevel_repr())
-    def __repr__(self):
+    def __repr__(self):  # pragma: no cover
         return "{:s}({!r})".format(self.__class__.__name__, self._lowlevel_repr())
 
     def __eq__(self, other):
         if other is self:
             return True
         if len(self) != len(other):
             return False
-        for v, wrap in zip(self, other):
-            if v != wrap:
+        for v1, v2 in zip(self, other):
+            if v1 != v2:
                 return False
         return True
 
@@ -850,18 +848,27 @@ def apply_conversion(k):
     def convert(k):
         if k is not None:
             if not isinstance(k, int):
-                raise TypeError("k must be int, got {} with value {}".format(type(k), k))
+                # This is not triggered in the current code because the outer
+                # layers protect against having to check here, but since the
+                # `convert` function is returned to the caller, let's be
+                # careful.
+                raise TypeError("k must be int, got {} with value {}".format(type(k), k))  # pragma: no cover
             # Almost standard semantics for negative indices. Usually -n < k < n,
             # but here we must allow for conversion of the end position, for
             # which the last valid value is one past the end.
-            if not -n <= k <= n:
-                raise IndexError("Should have -n <= k <= n, but n = len(args) = {}, and k = {}".format(n, k))
+            if n is not None and not -n <= k <= n:
+                raise IndexError("Should have -n <= k <= n, but n = {}, and k = {}".format(n, k))
             return apply_conversion(k) if k < 0 else k
     return convert
 
 def _canonize_slice(s, n=None, wrap=None):  # convert negatives, inject defaults.
     if not isinstance(s, slice):
-        raise TypeError("s must be slice, got {} with value {}".format(type(s), s))
+        # Not triggered in the current code, because this is an internal function
+        # and `in_slice` already checks; but let's be careful in case this is later
+        # used elsewhere. (And, it's already possible that some internal caller
+        # incorrectly uses the no-check mode of the internal implementation function
+        # `_index_in_slice`.)
+        raise TypeError("s must be slice, got {} with value {}".format(type(s), s))  # pragma: no cover
 
     step = s.step if s.step is not None else +1  # no "s.step or +1"; someone may try step=0
     if step == 0:
diff --git a/unpythonic/test/test_collections.py b/unpythonic/test/test_collections.py
@@ -8,10 +8,50 @@
 import threading
 
 from ..collections import (box, ThreadLocalBox, Some, Shim, unbox,
-                           frozendict, view, roview, ShadowedSequence, mogrify)
+                           frozendict, view, roview, ShadowedSequence, mogrify,
+                           in_slice, index_in_slice)
 from ..fold import foldr
+from ..llist import cons, ll
 
 def runtests():
+    with testset("internal utilities"):
+        test[in_slice(5, slice(10))]
+        test[in_slice(5, 5)]  # convenience: int instead of a slice
+        test[in_slice(5, slice(1, 10, 2))]
+        test[not in_slice(5, slice(0, 10, 2))]
+
+        test[index_in_slice(5, slice(10)) == 5]
+        test[index_in_slice(5, slice(1, 10, 2)) == 2]
+
+        # with sequence length parameter, and with negative indices
+        test[in_slice(8, slice(0, None, 1), 10)]
+        test[in_slice(-2, slice(0, None, 1), 10)]
+        test[in_slice(9, 9, 10)]  # convenience
+        test[in_slice(-1, -1, 10)]
+        test[in_slice(7, slice(None, None, -1), 10)]
+        test[in_slice(-3, slice(None, None, -1), 10)]
+        test[in_slice(7, slice(9, None, -1), 10)]
+        test[in_slice(-3, slice(9, None, -1), 10)]
+
+        test[in_slice(5, slice(0, None, 1), 10)]
+        test_raises[IndexError, in_slice(5, slice(0, None, 1), 3)]  # out of range when length = 3
+
+        test[index_in_slice(-1, slice(10), 10) == 9]
+        test[index_in_slice(-1, slice(None, None, -1), 10) == 0]
+        test[index_in_slice(7, slice(None, None, -2), 10) == 1]
+        test[index_in_slice(-3, slice(None, None, -2), 10) == 1]
+
+        test_raises[TypeError, in_slice("not an index", slice(10))]
+        test_raises[TypeError, in_slice(5, "not a slice or int")]
+        test_raises[TypeError, in_slice(1, slice(10), "not a length")]
+        test_raises[ValueError, in_slice(1, slice(10), -3)]  # negative length
+
+        test_raises[ValueError, in_slice(-1, slice(10))]  # need length to interpret negative indices
+
+        test_raises[ValueError, in_slice(1, slice(0, None, 0))]  # zero step
+        test_raises[ValueError, in_slice(1, slice(0, None, 1))]  # missing length for default stop with positive step
+        test_raises[ValueError, in_slice(1, slice(None, None, -1))]  # missing length for default start with negative step
+
     # box: mutable single-item container à la Racket
     with testset("box"):
         b = box(17)
@@ -76,6 +116,15 @@ def test_threadlocalbox_worker():
         t.join()
         test[unbox(tlb) == 42]  # In the main thread, this box still has the original value.
 
+        test[42 in tlb]
+        test[[x for x in tlb] == [42]]
+        test[tlb == 42]
+        test[len(tlb) == 1]
+
+        tlb2 = ThreadLocalBox(42)
+        test[tlb2 is not tlb]
+        test[tlb2 == tlb]
+
         # The default object can be changed.
         tlb = ThreadLocalBox(42)
         # We haven't sent any object to the box, so we see the default object.
@@ -116,6 +165,9 @@ def test_threadlocalbox_worker():
         test_raises[TypeError, s << 23]  # immutable
         test_raises[AttributeError, s.set(23)]
 
+        test[[x for x in s] == [42]]
+        test[len(s) == 1]
+
     # Shim (a.k.a. attribute proxy): redirect attribute accesses.
     #
     # The shim holds a box. Attribute accesses on the shim are redirected
@@ -181,6 +233,8 @@ class Zee:
             test[s.z == "hi from Zee"]
         test_chaining()
 
+        test_raises[TypeError, Shim("la la la")]  # not a box, shouldn't be able to Shim it
+
     # frozendict: immutable dictionary (like frozenset, but for dictionaries)
     with testset("frozendict"):
         d3 = frozendict({'a': 1, 'b': 2})
@@ -320,6 +374,18 @@ class Zee:
         test[v == [1, 2, 3, 4, 5]]
         test[lst == [42, 0, 1, 2, 3, 4, 5]]
 
+        tup = (1, 2, 3, 4, 5)
+        test_raises[TypeError, view(tup)]  # tuple is read-only, view is read/write
+
+        lst = list(range(5))
+        v = view(lst)[2:]
+        with test_raises(TypeError):
+            v[2, 3] = 42  # multidimensional indexing not supported
+        with test_raises(IndexError):
+            v[9001] = 42
+        with test_raises(IndexError):
+            v[-9001] = 42
+
     # read-only live view for sequences
     # useful to give read access to a sequence that is an internal detail
     with testset("roview"):
@@ -335,6 +401,24 @@ class Zee:
             v[2] = 3
         test_raises[AttributeError, v.reverse()]  # read-only view does not support in-place reverse
 
+        tup = tuple(range(5))
+        v1 = roview(tup)[2:]
+        test[v1 == v1]
+        v2 = roview(tup)[2:]
+        test[v2 is not v1]
+        test[v2 == v1]
+        v3 = roview(tup)[3:]
+        test[v3 != v1]
+        v4 = roview(tup)[0:2]
+        v5 = roview(tup)[1:3]
+        test[v5 != v4]
+
+        tup = tuple(range(5))
+        v1 = roview(tup)[2:]
+        test_raises[TypeError, v1[2, 3]]  # multidimensional indexing not supported
+        test_raises[IndexError, v1[9001]]
+        test_raises[IndexError, v1[-9001]]
+
     # sequence shadowing (like ChainMap, but only two levels, and for sequences, not mappings)
     with testset("ShadowedSequence"):
         tpl = (1, 2, 3, 4, 5)
@@ -343,6 +427,10 @@ class Zee:
         test[tpl == (1, 2, 3, 4, 5)]
         test[s[2:] == (42, 4, 5)]
 
+        test_raises[TypeError, s[2, 3]]  # multidimensional indexing not supported
+        test_raises[IndexError, s[9001]]
+        test_raises[IndexError, s[-9001]]
+
         s2 = ShadowedSequence(tpl, slice(2, 4), (23, 42))
         test[s2 == (1, 2, 23, 42, 5)]
         test[tpl == (1, 2, 3, 4, 5)]
@@ -357,6 +445,16 @@ class Zee:
         test[s2 == (1, 2, 23, 42, 5)]
         test[tpl == (1, 2, 3, 4, 5)]
 
+        with test_raises(TypeError):
+            ShadowedSequence(s4, "la la la", "new value")  # not a valid index specification
+
+        # no-op ShadowedSequence is allowed
+        s5 = ShadowedSequence(tpl)
+        test[s5[3] == 4]
+
+        s6 = ShadowedSequence(tpl, slice(2, 4), (23,))  # replacement too short...
+        test_raises[IndexError, s6[3]]  # ...which is detected here
+
     # mogrify: in-place map for various data structures (see docstring for details)
     with testset("mogrify"):
         double = lambda x: 2 * x
@@ -387,11 +485,16 @@ class Zee:
         test[mogrify(double, d.values()) == {4, 8, 12}]
         test[d == {1: 2, 3: 4, 5: 6}]
 
+        test[mogrify(double, cons(1, 2)) == cons(2, 4)]
+        test[mogrify(double, ll(1, 2, 3)) == ll(2, 4, 6)]
+
         b = box(17)
         b2 = mogrify(double, b)
         test[b2 == 34]
         test[b2 is b]
 
+        test[mogrify(double, Some(21)) == Some(42)]
+
         tup = (1, 2, 3)
         tup2 = mogrify(double, tup)
         test[tup2 == (2, 4, 6)]
