Implement condition/attr __repr__

[rajveerappan]

The main goal of this PR is to implement a human readable __repr__ representation of the boto3 DynamoDB conditions. The primary use case being for debugging and logging. Currently printing/logging a FilterExpression will show something like:

<boto3.dynamodb.conditions.AttributeExists object at 0x1041021f0>
<boto3.dynamodb.conditions.And object at 0x104102370>
<boto3.dynamodb.conditions.In object at 0x1041027f0>

After this change the printing/logging will display a more human readable version like:

attribute_exists(mykey)
(mykey = foo AND myotherkey = bar)
mykey IN foo

The way this is implemented is by turning an expression_format which looks like {0} {operator} {1} into something that looks like {values[0]} {operator} {values[1]} which can be passed to the string format method using named indices.

[tim-finnigan]

Linking to the issue you created: #3259

The team is open to improving the logging but mentioned that repr() should be used here rather than str().

[rajveerappan]

Hi @tim-finnigan, apologies for the late response, not sure why Github didn't notify me of your response. I'll make the change to implement repr() instead and add tests. Please let me know if there are any concerns around performance with using the REGEX the way I am here, if not I'll update this PR in the next day or two.

[rajveerappan]

@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.

[rajveerappan]

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

[tibbe]

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.

[tibbe]

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

[tibbe]

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.

[rajveerappan]

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?

[tibbe]

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)})'

[rshah-evertz]

Any updates on this?

[rajveerappan]

Any updates on this?

@tim-finnigan how can we get this PR merged?

[tim-finnigan]

Thanks for your patience — I don't have any updates here, a maintainer on the team will need to review this when there is bandwidth. I'll try raising this PR again but we can't guarantee any timelines on if or when any PR would get merged.