-
Notifications
You must be signed in to change notification settings - Fork 402
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
fix(idempotency): include decorated fn name in hash #869
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #869 +/- ##
========================================
Coverage 99.90% 99.90%
========================================
Files 118 118
Lines 5125 5126 +1
Branches 571 571
========================================
+ Hits 5120 5121 +1
Misses 2 2
Partials 3 3
Continue to review full report at Codecov.
|
@@ -124,7 +125,7 @@ def __init__(self): | |||
self._cache: Optional[LRUDict] = None | |||
self.hash_function = None | |||
|
|||
def configure(self, config: IdempotencyConfig) -> None: | |||
def configure(self, config: IdempotencyConfig, function_name: str) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@heitorlessa - should this be optional? or required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional. Only because it's a public method and we have no way of knowing if someone took a dependency on it via tests or something - if they did, we will break them and take another release to fix it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
two minor changes: 1/ make function_name
optional as we don't know who ended up taking a dependency on that public method (Hyrum's Law), 2/ use f-string to optimize memory and a CPU cycle as str
is more expensive in Python than usual
@@ -124,7 +125,7 @@ def __init__(self): | |||
self._cache: Optional[LRUDict] = None | |||
self.hash_function = None | |||
|
|||
def configure(self, config: IdempotencyConfig) -> None: | |||
def configure(self, config: IdempotencyConfig, function_name: str) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional. Only because it's a public method and we have no way of knowing if someone took a dependency on it via tests or something - if they did, we will break them and take another release to fix it
aws_lambda_powertools/utilities/idempotency/persistence/base.py
Outdated
Show resolved
Hide resolved
make configure backwards compatible resolve the full function name ahead of time
@heitorlessa @cakepietoast - i have pushed recommended changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot Mike!
Issue #, if available:
Description of changes:
Include the callable function name in the generated idempotent key
Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.