On Fri, 2017-04-07 at 06:10 +0000, Ankit Yadav wrote: > Hello Everyone, > > I have changed the compare function, took help from Entry.__eq__ function. > I have also updated the user_compare_test.py, Now it includes 4 assertions. > Details can be found in lib389/tests/idm/user_compare_test.py > Also removed the 'DeepDiff' dependency. > Please review this commit: > https://pagure.io/fork/ankity10/lib389/c/762ef9054f01837a33219aa2412d428f5d783c52?branch=ticket-1-config-compare > > Thanks
Hi there, This is a big improvement! Thanks for taking all my feedback. When we supply patches for review, we generally like to supply them as 1, so we can see the whole change in one, rather than over two commits. I've just updated our contributing guide, so it may be worth you reading over it. http://www.port389.org/docs/389ds/contributing.html It has a section on how to squash commits together in git, as well as our commit message formats we prefer. 21 + # check if RDN is same 22 + if obj1.rdn != obj2.rdn: 23 + return False I think the question is if we want to check if two objects in different subtrees have the same attributes, or if we want *true* object equality. *true* object equality, you want to check nsUniqueId first, because that's always going to be different between everything. But if we want to just compare arbitrary objects, I think the rdn check is better, and we need to ignore nsUniqueId. For this purpose perhaps we want the "loose" equality, just checking if attributes are the same. IE if I had: cn=user1,ou=a cn=user1,ou=b They should compare the same, even though their nsUniqueId differs. For now I think this code is okay, but we should leave some comments around this discussion there, and certainly document in the docstring that this is a loose equality, not a strict "this object *is* this other object". 46 # removing _compate_exclude attrs from all attrs 47 for key in self._compare_exclude: 48 - attrs_dict.pop(key.lower(), None) 49 + del attrs_dict[key.lower()] Rather than deleting these, why not use a set and do: compare_attrs = set(attrs_dict.keys) - set(self._compare_exclude) The issue with python is that sometimes delete and operations like that are destructive and have weird side effects, so it's better to take a functional approach and make a new set of attributes you want, rather than modifying a set you already have. 48 testuser1.delete() 49 testuser2.delete() The test teardown deletes the database, so you don't need to delete the users at the end :) If you squash this patch and the last one together now, and send again for a quick check, I think you would be pretty close to having a working object compare to merge :) Thanks heaps! -- Sincerely, William Brown Software Engineer Red Hat, Australia/Brisbane
signature.asc
Description: This is a digitally signed message part
_______________________________________________ 389-devel mailing list -- 389-devel@lists.fedoraproject.org To unsubscribe send an email to 389-devel-le...@lists.fedoraproject.org
