On 02.06.2016 16:16, Peter Lacko wrote:
Rebased with updated tests.

Peter

----- Original Message -----
From: "Martin Basti" <mba...@redhat.com>
To: "Peter Lacko" <pla...@redhat.com>
Cc: freeipa-devel@redhat.com
Sent: Thursday, June 2, 2016 1:50:06 PM
Subject: Re: [Freeipa-devel] [PATCH] 0002 New User Role Tests

Your patch doesn't apply anymore on master, there were changes in role
test and patch have to be updated and rebased

Please look at this for new changes in test_role_plugin.py
https://git.fedorahosted.org/cgit/freeipa.git/commit/?id=5f42b42bd4557a669ab5cfcf1af6596f1a2535f1

Martin^2


On 02.06.2016 11:49, Peter Lacko wrote:
Sorry for late response, I wasn't working these days.
Fixed patch is in attachment.

Peter


----- Original Message -----
From: "Martin Basti" <mba...@redhat.com>
To: "Peter Lacko" <pla...@redhat.com>, freeipa-devel@redhat.com
Sent: Monday, May 9, 2016 1:06:08 PM
Subject: Re: [Freeipa-devel] [PATCH] 0002 New User Role Tests



On 09.05.2016 13:04, Martin Basti wrote:
On 09.05.2016 12:19, Peter Lacko wrote:
+# pylint: disable=unicode-builtin
I'm not doing complete review, just the line above hit my eyes.

unicode() is not in Py3 because all strings there are unicode, thus
you cannot use it directly, you need something like

if six.PY2:
      str = unicode

and use str() everywhere and remove that #pylint line

FYI all enabled pylint checks are there for a good reason, be careful
with disabling it (mainly disabling it for a whole module) rather ask
before if you are not sure.

Martin^2

Nope, sorry, I temporarily forgot how to python

instead of pylint disable use this

if six.PY3:
       unicode =str

and keep unicode there. Sorry

Martin^2
Hi,

I don't have time to continue with full review, maybe somebody else can continue instead of me, but anyway NACK, because using str(unicode()) or unicode(str()) is bad in both ways, it should be just unicode() in case of strings (with correct mapping unicode=str in Py3). I just I noticed this by reading code, but I didn't do deeper review, so there might be more mistakes.

Martin^2

--
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