Check if sys.stderr has isatty by eggplants · Pull Request #1761 · RDFLib/rdflib (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation12 Commits3 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Closes: #1760
Proposed Changes:
- Check if
sys.stderrhasisattyinrdflib/__init__.py.
Check if sys.stderr has isatty (fix: #1760) Check if sys.stderr has isatty
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.
Member
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 sys.stderr has isatty() the logging is enabled?
Do I get it right, only in the case that sys.stderr has isatty() the logging is enabled?
Yes.
Test works fine even without the change, and as per our guide:
https://github.com/RDFLib/rdflib/blob/master/docs/developers.rst#tests
Tests that you add should show how your new feature or bug fix is doing what you say it is doing: if you remove your enhancement, your new tests should fail!
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 sys.stderr with None from the outside without changes to __init__.py? sys.stderr = None in the test seem to be reflected. Sorry for my stupidity.
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.
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.