Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance __repr__ method of Principal #275

Closed
wants to merge 1 commit into from

Conversation

martbab
Copy link
Contributor

@martbab martbab commented Nov 28, 2016

__repr__ now returns more descriptive string containing the actual principal
name while keeping the ability to reconstruct the object from it.

This makes principal names visible in debug logs, easing troubleshooting a
bit.

https://fedorahosted.org/freeipa/ticket/6505


def __repr__(self):
return "{}.{}('{}')".format(
self.__module__, self.__class__.__name__, self.__str__())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • does it actually work? Usually instances don't have a __module__ attribute, only classes.
  • don't call self.__str__()
  • where is the unit test? :)

Something like this should work

    def __repr__(self):
        return "{0.__module__}.{0.__name__}('{1}')".format(
            self.__class__, self)

Copy link
Member

@tiran tiran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please see inline comments

@martbab
Copy link
Contributor Author

martbab commented Nov 29, 2016

Sorry I somehow botched that, but it worked nevertheless. I have re-worked the PR according to your comments.

In [1]: import ipapython.kerberos
In [2]: p = ipapython.kerberos.Principal(u"HTTP/replica1.ipa.test")
In [3]: p
Out[3]: ipapython.kerberos.Principal('HTTP/replica1.ipa.test')
In [5]: r = eval('p')
In [6]: r
Out[6]: ipapython.kerberos.Principal('HTTP/replica1.ipa.test')
In [7]: r == p
Out[7]: True

@tiran
Copy link
Member

tiran commented Nov 29, 2016

Can you please add a test toipatests/test_ipapython/test_kerberos.py test_principals? Something along the line assert repr(princ) == "ipapython.kerberos.Principal('{}')".format(principal_name) should do the trick (untested).

`__repr__` now returns more descriptive string containing the actual principal
name while keeping the ability to reconstruct the object from it.

This makes principal names visible in debug logs, easing troubleshooting a
bit.

https://fedorahosted.org/freeipa/ticket/6505
@martbab
Copy link
Contributor Author

martbab commented Nov 29, 2016

That sound like a good idea. Added such assert to the unit tests.

@tiran tiran added the ack Pull Request approved, can be merged label Nov 29, 2016
@martbab
Copy link
Contributor Author

martbab commented Nov 30, 2016

@martbab martbab added the pushed Pull Request has already been pushed label Nov 30, 2016
@martbab martbab closed this Nov 30, 2016
@martbab martbab deleted the principal-repr branch November 30, 2016 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
2 participants