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 #139: replace stdout with end in show. #140

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

ben-e-whitney
Copy link

See #139. This commit reverts the changes made to Treelib.show in 4dde1fd and makes the changes proposed in this comment.

@ben-e-whitney
Copy link
Author

Would it be better for me to provide an implementation of contextlib.redirect_stdout or to avoid its use altogether? I don't know the policy on Python 2 compatibility.

print(self._reader)
else:
return self._reader
print('\n'.join(self._reader), end=end)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I strongly disagree with using print, see #141

Using print to stdout in a library is a bad practice, see for instance this article

It's also bad practice to ship a package that prints directly to stdout because it removes the user's ability to control the messages.

Copy link

Choose a reason for hiding this comment

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

This article talk about logging. I don't think It is related to this use case here. Here it is just an helper function.

You could create 2 functions, it would make the API simpler. I also think a stream as input is better, anyway the default value is stdout.

:return: None
"""
self._reader = ""
self._reader = []
Copy link
Collaborator

@leonardbinet leonardbinet Feb 22, 2020

Choose a reason for hiding this comment

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

IMO there is no reason to have a member variable as @GalacticStarfish explains it in #91. What do you think of replacing self._reader by a reader variable?

Copy link

Choose a reason for hiding this comment

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

+1, that's just a wast of memory cause it is cached but not reused AFAICS.

@leonardbinet
Copy link
Collaborator

@ben-e-whitney thanks for your contribution 👍, I approve converting the reader from string to list, but I still disagree with the use of print, @caesar0301 WDYT?

@caesar0301 caesar0301 requested a review from liamlundy as a code owner March 27, 2023 11:39
@caesar0301 caesar0301 force-pushed the dev branch 3 times, most recently from 83cded4 to 545e05b Compare March 28, 2023 04:49
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.

3 participants