craig-rueda commented on pull request #14109: URL: https://github.com/apache/superset/pull/14109#issuecomment-820662503
> 1. Could you elaborate how is this PR related to #13817? > 2. I'm not sure what is the additional benefits of adding a factory class. If the goal is to abstract away the access to `config['SECRET']` and provide a way to generate encrypted field differently. What's preventing us from making this more functional? > > E.g. > > In `config.py`: > > ```python > from flask import current_app > > SQLA_ENCRYPTED_FIELD_FACTORY = lambda col, **kwargs: EncryptedType( > col, current_app.config["SECRET_KEY"], **kwargs > ) > ``` > > Then add a new module `superset.models.sqla_types.encrypt` > > ``` > def create_encrypted_field(col, **kwargs): > return app.config["SQLA_ENCRYPTED_FIELD_FACTORY"](col, **kwargs) > ``` Thanks for your code suggestions! 1. I don't think it is related to that PR 2. There's always many ways to do something. I prefer being more concrete when defining patterns such as this. The Class/object based approach lets us define a contract for how an "adapter" should behave and provides a default implementation around it. Lastly, I'm trying to avoid direct references to `app.config` inside of Model classes via this intermediary (the factory itself). We have had to jump through several hoops because of the hard dependency on `config` in several layers where it should no exist. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
