Hi, thanks for updating the proposal!
Some further remarks:
- It may be a matter of taste, but I don't see how using Exceptions makes it
prettier ;-). If you don't like using an `assert` you can directly print the
error and exit but there's no need for a try/except block, is there?
- os.environ acts like a dict so you can test for the presence of a key with:
key in os.environ
or use get() to obtain a default None value if the key is missing:
value = os.environ.get(key)
without needing a try/except for KeyError either.
- Regarding Thu's comment, the problem is that you have not defined a list nor
a tuple, but simply a string. Have a look at the second part of this section of
the Python doc for details: [1]. Or try this in a Python prompt, and you'll
understand:
>>> a = ('admin_passwd') # just a plain string, not a tuple
>>> 'a' in a, type(a)
(True, <type 'str'>)
>>> a = ('admin_passwd',) # notice the trailing comma to define a tuple!
>>> 'a' in a, type(a)
(False, <type 'tuple'>)
>>>
- you need more options in that blacklist, at least ['admin_passwd',
'db_password', 'smtp_password']
- defining a named constant for the blacklist would have the advantage of
making it self-explaining (for readability, again)
I know some of these things may look like tiny details, but if we don't discuss
best practices when reviewing merge proposals, we never will... :-)
[1] http://docs.python.org/tutorial/datastructures.html#tuples-and-sequences
--
https://code.launchpad.net/~openerp-dev/openobject-server/trunk-environment-vars-cto/+merge/94564
Your team OpenERP R&D Team is subscribed to branch
lp:~openerp-dev/openobject-server/trunk-environment-vars-cto.
_______________________________________________
Mailing list: https://launchpad.net/~openerp-dev-gtk
Post to : [email protected]
Unsubscribe : https://launchpad.net/~openerp-dev-gtk
More help : https://help.launchpad.net/ListHelp
