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.

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

improv: Added __eq__ function to DictWrapper for better equality checks #233

Conversation

GroovyDan
Copy link
Contributor

Issue #, if available:

Description of changes:

This change is to allow easier testing of data classes by adding an eq function to the base DictWrapper class. This new function will check the equality of the data contained in the object opposed to the memory location.

Checklist

Breaking change checklist

RFC issue #:

  • Migration process documented
  • Implement warnings (if it can live side by side)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@heitorlessa
Copy link
Contributor

hey @GroovyDan thanks a lot for taking the time in making your first contribution, we appreciate it very much :)

LGTM at first glance and can see how it can make tests easier (it'd be great to have one test updated to cover it too). However, I need to triple check the side effects of not having __hash__, as this will make the class not hashable too.

what do you think @michaelbrewer?

Copy link
Contributor

@Nr18 Nr18 left a comment

Choose a reason for hiding this comment

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

@GroovyDan nice addition indeed, it did raise a question while looking at it though. (Next to the missing tests that @heitorlessa mentioned)

aws_lambda_powertools/utilities/data_classes/common.py Outdated Show resolved Hide resolved
Copy link
Contributor

@heitorlessa heitorlessa left a comment

Choose a reason for hiding this comment

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

One small change required as per @Nr18 pointed out

I've also double checked about equality and hash theory. __hash__ won't be necessary because Dicts are not hashable, so DictWrapper not being hashable with __eq__ isn't an issue in this case.

aws_lambda_powertools/utilities/data_classes/common.py Outdated Show resolved Hide resolved
Co-authored-by: Joris Conijn <jco@woodwing.com>
Copy link
Contributor

@heitorlessa heitorlessa left a comment

Choose a reason for hiding this comment

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

Thanks a lot for making the change @GroovyDan - Merging this now.

@heitorlessa heitorlessa merged commit bf4ef14 into aws-powertools:develop Dec 7, 2020
@Nr18 Nr18 mentioned this pull request Dec 7, 2020
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants