Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement condition/attr __repr__ #3254

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

Implement condition/attr __repr__ #3254

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2fb6383
Implement condition __str__
rajveerappan May 4, 2022
4a19dec
Switch to __repr__
rajveerappan Mar 7, 2023
b991f52
Update test_conditions.py
rajveerappan Mar 7, 2023
dd9e4fd
Update test_conditions.py
rajveerappan Mar 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions boto3/dynamodb/conditions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
)

ATTR_NAME_REGEX = re.compile(r'[^.\[\]]+(?![^\[]*\])')
EXPR_STR_FORMAT_REGEX = re.compile(r"\{(\d+)\}")


class ConditionBase:
Expand Down Expand Up @@ -59,6 +60,14 @@ def __eq__(self, other):
def __ne__(self, other):
return not self.__eq__(other)

def __repr__(self) -> str:
format_str = EXPR_STR_FORMAT_REGEX.sub(
r"{values[\1]}", self.expression_format
)
return format_str.format(
values=self._values, operator=self.expression_operator
)


class AttributeBase:
def __init__(self, name):
Expand Down Expand Up @@ -132,6 +141,9 @@ def __eq__(self, other):
def __ne__(self, other):
return not self.__eq__(other)

def __repr__(self) -> str:
return self.name


class ConditionAttributeBase(ConditionBase, AttributeBase):
"""This base class is for conditions that can have attribute methods.
Expand Down
81 changes: 81 additions & 0 deletions tests/unit/dynamodb/test_conditions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,10 @@ def test_eq(self):
},
)

def test_eq_repr(self):
actual_repr = str(Equals(self.value, self.value2))
assert actual_repr == 'mykey = foo'
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tim-finnigan / @tibbe I could use your input on these tests before I flesh them out fully. One option is what I'm doing here and adding a bunch of test_op_repr tests. Another option is to change ConditionBase#get_expression so that it returns a new repr key with the string representation and then update the existing test_op test cases to assert on the repr also.

I don't have a preference either way but let me know what you would prefer.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually I already fleshed them out but happy to revise based on comments. otherwise I think this PR is ready for review.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late reply. We might need to distinguish between what __str__ and __repr__ does. According to the specification of __repr__:

If at all possible, this should look like a valid Python expression that could be used to recreate an object with the same value (given an appropriate environment). If this is not possible, a string of the form <...some useful description...> should be returned. The return value must be a string object. If a class defines __repr__() but not __str__(), then __repr__() is also used when an “informal” string representation of instances of that class is required.

In the case of __repr__ we probably want something that looks like constructor application e.g. And(AttributeExists("x"), AttributeExists("y")). For __str__ we could then have whatever pretty-prints nicely e.g. the syntax actually used in DynamoDB query expressions.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I ultimately care about is that when the Stubber compares two expressions and they are non-equal it prints something useful. It currently prints using __str__ (which falls back to __repr__ if the condition class doesn't define it): https://github.com/boto/botocore/blob/9f1dd1c843cb938ee96af969045b8a7a0c53deee/botocore/stub.py#L386

Copy link

@tibbe tibbe Mar 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be concrete, I suggest we implement both __str__ and __repr__, using the implementation you have for __repr__ in this PR for __str__ and adding my suggestion for __repr__ above.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had originally started with __str__ but was asked to use __repr__ instead here:
#3254 (comment)

tbh I'm not particular as long as we can get this PR merged and have human understandable representations so that debugging is a nicer experience.

@tim-finnigan what can I do to help get this PR merged?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we go with a textual description the string should be surrounded by <>, as per the documentation of __report__(), or otherwise it will be hard to parse when there is surrounding text.

In this case I think we could actually get a real __repr__() (i.e. one fulfilling "If at all possible, this should look like a valid Python expression that could be used to recreate an object with the same value (given an appropriate environment).") by instead implementing __repr__() along the lines of:

def __repr__(self):
  return f'{self.__class__.__name__}({", ".join(self._values)})'


def test_ne(self):
self.build_and_assert_expression(
NotEquals(self.value, self.value2),
Expand All @@ -294,6 +298,10 @@ def test_ne(self):
},
)

def test_ne_repr(self):
actual_repr = str(NotEquals(self.value, self.value2))
assert actual_repr == 'mykey <> foo'

def test_lt(self):
self.build_and_assert_expression(
LessThan(self.value, self.value2),
Expand All @@ -304,6 +312,10 @@ def test_lt(self):
},
)

def test_lt_repr(self):
actual_repr = str(LessThan(self.value, self.value2))
assert actual_repr == 'mykey < foo'

def test_lte(self):
self.build_and_assert_expression(
LessThanEquals(self.value, self.value2),
Expand All @@ -314,6 +326,10 @@ def test_lte(self):
},
)

def test_lte_repr(self):
actual_repr = str(LessThanEquals(self.value, self.value2))
assert actual_repr == 'mykey <= foo'

def test_gt(self):
self.build_and_assert_expression(
GreaterThan(self.value, self.value2),
Expand All @@ -324,6 +340,10 @@ def test_gt(self):
},
)

def test_gt_repr(self):
actual_repr = str(GreaterThan(self.value, self.value2))
assert actual_repr == 'mykey > foo'

def test_gte(self):
self.build_and_assert_expression(
GreaterThanEquals(self.value, self.value2),
Expand All @@ -334,6 +354,10 @@ def test_gte(self):
},
)

def test_gte_repr(self):
actual_repr = str(GreaterThanEquals(self.value, self.value2))
assert actual_repr == 'mykey >= foo'

def test_in(self):
cond = In(self.value, (self.value2))
self.build_and_assert_expression(
Expand All @@ -346,6 +370,10 @@ def test_in(self):
)
assert cond.has_grouped_values

def test_in_repr(self):
actual_repr = str(In(self.value, self.value2))
assert actual_repr == 'mykey IN foo'

def test_bet(self):
self.build_and_assert_expression(
Between(self.value, self.value2, 'foo2'),
Expand All @@ -356,6 +384,10 @@ def test_bet(self):
},
)

def test_bet_repr(self):
actual_repr = str(Between(self.value, self.value2, 'foo2'))
assert actual_repr == 'mykey BETWEEN foo AND foo2'

def test_beg(self):
self.build_and_assert_expression(
BeginsWith(self.value, self.value2),
Expand All @@ -366,6 +398,10 @@ def test_beg(self):
},
)

def test_beg_repr(self):
actual_repr = str(BeginsWith(self.value, self.value2))
assert actual_repr == 'begins_with(mykey, foo)'

def test_cont(self):
self.build_and_assert_expression(
Contains(self.value, self.value2),
Expand All @@ -376,6 +412,10 @@ def test_cont(self):
},
)

def test_cont_repr(self):
actual_repr = str(Contains(self.value, self.value2))
assert actual_repr == 'contains(mykey, foo)'

def test_ae(self):
self.build_and_assert_expression(
AttributeExists(self.value),
Expand All @@ -386,6 +426,10 @@ def test_ae(self):
},
)

def test_ae_repr(self):
actual_repr = str(AttributeExists(self.value))
assert actual_repr == 'attribute_exists(mykey)'

def test_ane(self):
self.build_and_assert_expression(
AttributeNotExists(self.value),
Expand All @@ -396,6 +440,10 @@ def test_ane(self):
},
)

def test_ane_repr(self):
actual_repr = str(AttributeNotExists(self.value))
assert actual_repr == 'attribute_not_exists(mykey)'

def test_size(self):
self.build_and_assert_expression(
Size(self.value),
Expand All @@ -406,6 +454,10 @@ def test_size(self):
},
)

def test_size_repr(self):
actual_repr = str(Size(self.value))
assert actual_repr == 'size(mykey)'

def test_size_can_use_attr_methods(self):
size = Size(self.value)
self.build_and_assert_expression(
Expand All @@ -417,6 +469,10 @@ def test_size_can_use_attr_methods(self):
},
)

def test_size_eq_repr(self):
actual_repr = str(Size(self.value).eq(self.value2))
assert actual_repr == 'size(mykey) = foo'

def test_size_can_use_and(self):
size = Size(self.value)
ae = AttributeExists(self.value)
Expand All @@ -429,6 +485,10 @@ def test_size_can_use_and(self):
},
)

def test_size_and_ae_repr(self):
actual_repr = str(Size(self.value) & AttributeExists(self.value))
assert actual_repr == '(size(mykey) AND attribute_exists(mykey))'

def test_attribute_type(self):
self.build_and_assert_expression(
AttributeType(self.value, self.value2),
Expand All @@ -439,6 +499,10 @@ def test_attribute_type(self):
},
)

def test_attribute_type_repr(self):
actual_repr = str(AttributeType(self.value, self.value2))
assert actual_repr == 'attribute_type(mykey, foo)'

def test_and(self):
cond1 = Equals(self.value, self.value2)
cond2 = Equals(self.value, self.value2)
Expand All @@ -452,6 +516,12 @@ def test_and(self):
},
)

def test_and_repr(self):
cond1 = Equals(self.value, self.value2)
cond2 = Equals(self.value, self.value2)
actual_repr = str(And(cond1, cond2))
assert actual_repr == '(mykey = foo AND mykey = foo)'

def test_or(self):
cond1 = Equals(self.value, self.value2)
cond2 = Equals(self.value, self.value2)
Expand All @@ -465,6 +535,12 @@ def test_or(self):
},
)

def test_or_repr(self):
cond1 = Equals(self.value, self.value2)
cond2 = Equals(self.value, self.value2)
actual_repr = str(Or(cond1, cond2))
assert actual_repr == '(mykey = foo OR mykey = foo)'

def test_not(self):
cond = Equals(self.value, self.value2)
not_cond = Not(cond)
Expand All @@ -477,6 +553,11 @@ def test_not(self):
},
)

def test_not_repr(self):
cond = Equals(self.value, self.value2)
actual_repr = str(Not(cond))
assert actual_repr == '(NOT mykey = foo)'


class TestConditionExpressionBuilder(unittest.TestCase):
def setUp(self):
Expand Down