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

Check if sys.stderr has isatty #1761

Merged
merged 3 commits into from
Apr 2, 2022
Merged

Check if sys.stderr has isatty #1761

merged 3 commits into from
Apr 2, 2022

Conversation

eggplants
Copy link
Contributor

@eggplants eggplants commented Mar 15, 2022

Closes: #1760

Proposed Changes:

  • Check if sys.stderr has isatty in rdflib/__init__.py.

@eggplants eggplants changed the title Check if sys.stderr has isatty (fix: #1760) Check if sys.stderr has isatty Mar 15, 2022
@aucampia
Copy link
Member

Please include tests.

@aucampia
Copy link
Member

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.

Copy link
Member

@aucampia aucampia left a 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.

@white-gecko
Copy link
Member

Do I get it right, only in the case that sys.stderr has isatty() the logging is enabled?

@eggplants
Copy link
Contributor Author

eggplants commented Mar 25, 2022

Do I get it right, only in the case that sys.stderr has isatty() the logging is enabled?

Yes.

@aucampia
Copy link
Member

aucampia commented Mar 27, 2022

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.

@eggplants
Copy link
Contributor Author

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.

@aucampia
Copy link
Member

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.

https://github.com/python/cpython/blob/d4a93e4de17ed8c2334fc72ca4cee737c3b738ec/Python/bltinmodule.c#L1871-L1873

        /* sys.stdout may be None when FILE* stdout isn't connected */
        if (file == Py_None)
            Py_RETURN_NONE;

@aucampia
Copy link
Member

@nicholascar nicholascar self-requested a review March 30, 2022 14:41
Copy link
Member

@nicholascar nicholascar left a 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.

@aucampia
Copy link
Member

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.

@aucampia
Copy link
Member

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.

@aucampia aucampia merged commit a0632b8 into RDFLib:master Apr 2, 2022
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.

Issues with installing rdflib
4 participants