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

fix for pprint #660 #661

Merged
merged 3 commits into from
Feb 7, 2017
Merged

fix for pprint #660 #661

merged 3 commits into from
Feb 7, 2017

Conversation

vnitinv
Copy link
Contributor

@vnitinv vnitinv commented Feb 6, 2017

Now print print and pprint should work as expected.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 98.726% when pulling f84a329 on vnitinv:master into 9ebe7df on Juniper:master.

@coveralls
Copy link

coveralls commented Feb 6, 2017

Coverage Status

Coverage decreased (-0.1%) to 98.966% when pulling f61a68c on vnitinv:master into 9ebe7df on Juniper:master.

Copy link
Contributor

@stacywsmith stacywsmith left a comment

Choose a reason for hiding this comment

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

I like the idea, but I believe the implementation can be improved per my comments.

@@ -192,7 +193,7 @@ def __len__(self):
"""
return len(self._callbacks)

def __repr__(self):
def __str__(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on these definitions, I believe these two function definitions should be reversed.
__str__() should be the formatted and nicely printable representation.
https://docs.python.org/2/library/functions.html#str
__repr__() should be the string which could be passed back to eval()
https://docs.python.org/2/library/functions.html#repr

Copy link
Contributor Author

@vnitinv vnitinv Feb 7, 2017

Choose a reason for hiding this comment

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

"Based on these definitions, I believe these two function definitions should be reversed.
str() should be the formatted and nicely printable representation."

I kept it normal as earlier too facts used to be printed in a line only. If we want it to be formated, then we need only repr.

"repr() should be the string which could be passed back to eval()
https://docs.python.org/2/library/functions.html#repr"
The kind of facts we have(containing objects inside dict), that cant be EVALuated.
For me it use to throw SyntexError when used eval with fats sring.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the string from repr() can not yet be passed to eval() successfully, but that's because of the string representation of the jnpr.junos.version_info objects. We can fix that when I address #651. If we keep the function the __repr__() function as it was, and add your new function as __str__(), then we most closely mimic the previous behavior when dev.facts was a true dict.

for key in sorted(self):
if not key.startswith('_'):
self.get(key)
return pformat(self._cache)
Copy link
Contributor

Choose a reason for hiding this comment

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

Your implementation causes the "internal" facts (the facts whose names begin with an underscore) to also be printed. In addition, I believe the whole method implementation can be reduced to:

return pformat(dict(self))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Stacy, This is even what I wanted.

Copy link
Contributor Author

@vnitinv vnitinv left a comment

Choose a reason for hiding this comment

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

Changed as per Stacy feedback. Still need to decide if we need bot str and repr

@coveralls
Copy link

coveralls commented Feb 7, 2017

Coverage Status

Coverage decreased (-0.03%) to 99.044% when pulling adffd68 on vnitinv:master into 9ebe7df on Juniper:master.

Copy link
Contributor

@stacywsmith stacywsmith left a comment

Choose a reason for hiding this comment

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

I discussed further with @vnitinv and we agreed that the current implementation in this pull request most closely mimics the previous behavior. Therefore, we should NOT switch the __repr__() and __str__() functions as I had previously suggested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants