On 05/27/2015 08:17 PM, Martin Basti wrote:
On 27/05/15 19:27, Rob Crittenden wrote:
Martin Basti wrote:


Thank you.

I haven't finished review yet, but I have few notes in case you will
modify the patch.

Please fix following issues:


3)
There are many PEP8 errors, can you fix some of them,?

Is PEP8 a concern? What kinds of errors do we fix? For example, the
current model for defining options generates a slew of indention errors.

In old modules it's preferred to keep the old indentation style for options(not to mix 2 styles). New modules should use following pep8 compliant style:
        Str(
            'cn',
            cli_name='name',
            primary_key=True,
            label=_('Server name'),
            doc=_('IPA server hostname'),
        ),

We try to keep PEP8 in new code, mainly indentation, blank lines, too
long lines.
Yes in test definitions and option definitions, is better to keep the
same style, but other parts of code should be PEP8.

For example these should be fixed
./ipatests/test_xmlrpc/test_serviceconstraint_plugin.py:37:13: E225
missing whitespace around operator
./ipatests/test_xmlrpc/test_serviceconstraint_plugin.py:39:1: E302
expected 2 blank lines, found 1
./ipatests/test_xmlrpc/test_serviceconstraint_plugin.py:42:1: E302
expected 2 blank lines, found 1



I'll wait and see what falls out of the API review before making any
real changes.

rob


Martin^2



--
Petr Vobornik

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to