Skip to content

feat(parameters): Allow callable transforms #894

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

Closed

Conversation

whardier
Copy link
Contributor

@whardier whardier commented Dec 13, 2021

Issue #, if available:

#893

Description of changes:

Add in changes to base parameter classes and feature flags appconfig store to allow for callable transforms to be used as well as the available stringy references.

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.

@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 13, 2021
@whardier whardier changed the title Feat/callable transforms feat(parameters): Allow callable transforms Dec 13, 2021
@ran-isenberg
Copy link
Contributor

I'm not that having a parameter as both str and callable is the best approach. maybe have it is as string, set to custom and add another param for the callable?
also, what about error handling? what happens if the transform raises an exception?

@whardier
Copy link
Contributor Author

I don't like it either but I am working within a compatible solution. Ideally transform would be an instance of a transform object with a .transform method and a list of compatible key suffixes.

Error handling is the same as the json and binary methods. all exceptions lead to None unless transform errors are set to raise.

@whardier
Copy link
Contributor Author

perhaps the best approach is to register transform classes by string an key suffix and then do a lookup and execute them generically.

@heitorlessa
Copy link
Contributor

@whardier we appreciate the bias for action in implementing a feature you need, however, we'd like to hear from more customers that this is necessary & requirements before diving into the implementation where we can iron out maintainability, etc.

Locking this PR for now until we hear from more customers

@aws-powertools aws-powertools locked and limited conversation to collaborators Dec 14, 2021
@heitorlessa heitorlessa added need-customer-feedback Requires more customers feedback before making or revisiting a decision revisit-in-3-months Requires more customers feedback before making or revisiting a decision labels Dec 14, 2021
@heitorlessa heitorlessa closed this Feb 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
do-not-merge need-customer-feedback Requires more customers feedback before making or revisiting a decision revisit-in-3-months Requires more customers feedback before making or revisiting a decision size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants