[Freeipa-devel] [PATCHES] 0052-0055 Separate master and forward DNS zones to separate objectClasses

[Petr Vobornik]

On 18.6.2014 13:42, Martin Basti wrote:
> Rebased patches with pep8 fixes attached

git diff HEAD~4 -U0 | pep8 --diff --ignore=E501,E126,E128,E124
./ipalib/plugins/dns.py:1754:9: E265 block comment should start with '# '
./ipalib/plugins/dns.py:1755:9: E265 block comment should start with '# '
./ipalib/plugins/dns.py:2550:13: E265 block comment should start with '# '
./ipalib/plugins/dns.py:2551:16: E713 test for membership should be 'not in'
./ipalib/plugins/dns.py:3516:9: E265 block comment should start with '# '
./ipalib/plugins/dns.py:3686:12: E713 test for membership should be 'not in'

I don't like how the patches are structured. It's hard to review. IMHO 
you should have started with creation of the base class and then build 
the forward zone on top of it. Reading a bunch of copy-pasted code with 
minor changes which is then removed in the next patch is a waste of 
time.  Current approach is acceptable if the patch set is huge and 
rebases would be really difficult. Or if it's sent and reviewed 
separately. But what's done is done.

1. Is it possible to disable the interactive wizard for dnsrecord-add 
forward.zone.? It would be nice to show the error right at the beginning.

Patch 54-5:
2. You forgot to remove the 'execute' method from 'dnszone_disable' and 
'dnszone_enable'

3. 'pre_callback' in 'dnsforwardzone_find' seems to be redundant, it 
just calls the parent's pre_callback with no additional logic


General question:

Looking at the help text, especially the `--name=DNSNAMEPARAM`, I wonder 
if have somewhere documented the formats of various param types.
-- 
Petr Vobornik