Fix plone.memoize.view to support unhashable types in function arguments #37
Conversation
|
@thet thanks for creating this Pull Request and helping to improve Plone! TL;DR: Finish pushing changes, pass all other checks, then paste a comment: To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically. Happy hacking! |
|
@jenkins-plone-org please run jobs |
thet
force-pushed
the
fix-hashable-arguments
branch
from
8fbaaf1
to
4e9cea7
Compare
|
@davisagli a function is such a hashable but non-JSON serializable type... Is this a relevant example? Are there other types which would fail with my fix? |
|
@thet a datetime...any data that is not supported by the default json serializer. I think it'd be wiser to add handling for some specific types like list and dict (i.e. convert to tuple and use dict.items()) rather than use the json module. Even then there are edge cases to consider, like whether 2 dicts in different order should be considered equal, etc... |
| args[1:], | ||
| frozenset(kwargs.items()), | ||
| json.dumps(args[1:]), | ||
| json.dumps(kwargs), |
There was a problem hiding this comment.
While this change allows unhashable types it prevents using types which are not JSON serialisable, like datetime.date. So this change might break existing code.
|
@davisagli @icemac tnx for the reviews! Yes, existing code must not be broken. I'll provide an improved version of this PR. |
Fixes: #36