Skip to content

Commit

Permalink
BUG: remove all identity predicates
Browse files Browse the repository at this point in the history
When Choices was changed to disallow an empty set, it introduced a bug
where `Str >= Str % Choices({...})` would raise an error instead of
reporting `True`. This expression never really happens in practice, so
it doesn't impact much of anything. But now there are more generic
protections preventing a plugin developer from registering an empty
predicate, which is probably not what they intended.
  • Loading branch information
ebolyen authored and thermokarst committed Dec 11, 2017
1 parent ee15f69 commit 684b8b7
Show file tree
Hide file tree
Showing 3 changed files with 14 additions and 10 deletions.
15 changes: 9 additions & 6 deletions qiime2/core/type/grammar.py
Expand Up @@ -343,7 +343,10 @@ def to_ast(self):

class Predicate(_TypeBase):
def __init__(self, *args, **kwargs):
self._truthy = any(map(bool, args)) or any(map(bool, kwargs.values()))
truthy = any(map(bool, args)) or any(map(bool, kwargs.values()))
if not truthy:
raise TypeError("Predicate %r has no arguments or cannot "
"constrain the type." % self.__class__.__name__)

self._freeze_()

Expand All @@ -362,17 +365,17 @@ def __contains__(self, value):
def _is_element_(self, value):
return True

def __bool__(self):
return self._truthy

def __le__(self, other):
if other is None:
other = self.__class__()
# The definition of a predicate type is some constraint. So if
# there isn't a predicate, then `self` must be a subset. There are
# no "identity" predicates which would complicate this.
return True
return self._is_subtype_(other)

def __ge__(self, other):
if other is None:
other = self.__class__()
return False
return other._is_subtype_(self)

def _aug_is_subtype(self, other):
Expand Down
3 changes: 2 additions & 1 deletion qiime2/core/type/primitive.py
Expand Up @@ -75,7 +75,8 @@ def __init__(self, *args, inclusive_start=_RANGE_DEFAULT_INCLUSIVE_START,
self.inclusive_start = inclusive_start
self.inclusive_end = inclusive_end

super().__init__(args)
truthy = self.start is not None or self.end is not None
super().__init__(truthy)

def __hash__(self):
return (hash(type(self)) ^
Expand Down
6 changes: 3 additions & 3 deletions qiime2/core/type/tests/test_grammar.py
Expand Up @@ -196,7 +196,7 @@ def test_hashable(self):
a = grammar.TypeExpression('X')
b = grammar.TypeExpression('Y', fields=(a,))
c = grammar.TypeExpression('Y', fields=(a,))
d = grammar.TypeExpression('Z', predicate=grammar.Predicate())
d = grammar.TypeExpression('Z', predicate=grammar.Predicate("stuff"))

self.assertIsInstance(a, collections.Hashable)
# There really shouldn't be a collision between these:
Expand Down Expand Up @@ -435,8 +435,8 @@ def test_mod_w_existing_predicate(self):
with self.assertRaisesRegex(TypeError, 'predicate'):
X % grammar.Predicate('Other')

def test_mod_w_falsy_predicate(self):
X = grammar.TypeExpression('X', predicate=grammar.Predicate())
def test_mod_w_none_predicate(self):
X = grammar.TypeExpression('X', predicate=None)
predicate = grammar.Predicate("Truthy")
self.assertIs((X % predicate).predicate, predicate)

Expand Down

0 comments on commit 684b8b7

Please sign in to comment.