WIP: User LDAP setter/getter methods for each config key#22
WIP: User LDAP setter/getter methods for each config key#22danil-topchiy wants to merge 2 commits intoEnterpriseyIntranet:masterfrom
Conversation
danil-topchiy
force-pushed
the
user_ldap_refactor
branch
from
589ebce to
6236346
Compare
matejak
left a comment
matejak
left a comment
There was a problem hiding this comment.
Hello, thank you for addressing this! As I mention in the comment, I am not in favor of the exec way how to do this, but the other aspect of the solution you chose are great.
NextCloud.py
Outdated
|
|
||
| # create and add getter method | ||
| getter_name = "get_ldap_{}".format(key_name) | ||
| exec( |
There was a problem hiding this comment.
I don't like the exec here. I propose to do something else - you probably want to have a generic (maybe private) getter/setter that accepts the key as well, and you somehow just stuff the class/instance with wrappers that behave in a way that e.g. get_ldap_user_filter calls _get_ldap_property("user_filter").
You can use lambda to do the wrap, and you can even add those methods to the class itself (i.e. in the module-level code, not in the class constructor). However, watch out for closures when working with lambda.
There was a problem hiding this comment.
Thanks for suggestions! Yes, using exec was bad idea. I refactored it: moved to module level and attaching methods to class and rewritten exec to functions. Although, I used def functions, not lambdas for explicity, is that ok ?
Also, I left get_ldap_config, edit_ldap_config not private and open for user in case if somebody will need to see full config view and make bulk update of keys. As well use them in config keys setters/getters.
|
The inspection completed: 3 updated code elements |
|
Rebased in #25 |
3 participants
As was discussed in #17