-
Notifications
You must be signed in to change notification settings - Fork 564
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
Check if sys.stderr has isatty #1761
Conversation
Please include tests. |
@eggplants Looking at the issue this is trying to I think I can accept this without tests, however tests always makes it an easier choice to approve. I would like to see this code being removed though at some point in time, logging setup should be left to the user of the library, as with this approach it is likely that the setup is not as users want it, but I guess while it is there best to make it not break. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving but will wait for at least one more review since there are no tests and will really appreciate some tests here to make sure we can actually reproduce the issue and ensure it is fixed.
Do I get it right, only in the case that |
Yes. |
Test works fine even without the change, and as per our guide: https://github.com/RDFLib/rdflib/blob/master/docs/developers.rst#tests
I will look at adding tests when I have time but it will be some weeks I think, as there are other things I think are higher priority. |
Is there a way to overwrite |
I think the best way to test it is to create a new python process with stdout closed, which will result in stdout being null as far as I understand, though I'm not entirely sure. /* sys.stdout may be None when FILE* stdout isn't connected */
if (file == Py_None)
Py_RETURN_NONE; |
The other option is to use one of: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving but can't merge until Drone is working again and all tests are seen to pass.
This reverts commit 9e0d0b6.
Removed the test file as it is out of sync with master branch organization and it passes even without this PR, can be added in another PR if there is something else it tests for. |
CI passed now, again the test is removed as it passes with or without this change, I will merge tonight still if there are no objections. I think this change is safe enough. |
Closes: #1760
Proposed Changes:
sys.stderr
hasisatty
inrdflib/__init__.py
.