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: remove log message on import #1398

Merged
merged 3 commits into from Sep 13, 2021

Conversation

donbowman
Copy link
Contributor

Fixes #839.

Its not appropriate to print messages on import
of a library.

Fixes #

Proposed Changes

Fixes RDFLib#839.

Its not appropriate to print messages on import
of a library.
@nicholascar
Copy link
Member

@donbowman I agree with the sentiment of your change but I'm going to alter the PR to tidy tings a bit further!

@nicholascar
Copy link
Member

@donbowman how about this now? It still logs RDFlib version, but only in so-called 'interactive mode' when imported via terminal (line 102)

@ashleysommer
Copy link
Contributor

I'm sure there has been more than one issue created about this in the past, and at least one other PR to address it.
I wonder what ever came of those issues and PRs, I can't seem to find them now (it's a hard issue to search for in github issues).
I remember at least one other PR addressed this by only issuing the log in interactive mode, same as you've done here.

@nicholascar
Copy link
Member

I wonder what ever came of those issues and PRs

I thought I remembered a fix too... Oh well, addressed now.

@nicholascar
Copy link
Member

@ashleysommer just adding you to review in case I made a code boo boo. If @donbowman is happy with my changes and you approve, we can merge

@donbowman
Copy link
Contributor Author

donbowman commented Sep 5, 2021

@donbowman how about this now? It still logs RDFlib version, but only in so-called 'interactive mode' when imported via terminal (line 102)

I don't agree. I don't want to see output to stdout from libraries, regardless of how I'm using this. E.g.. if I wrote a curses program this would wreck it.

If you want debugging available, use the logger.
If you want to know the version, make a variable the caller can check.

But to print in a lib I don't think is good practice.

@nicholascar
Copy link
Member

OK, I've removed the logging of the version

@ashleysommer
Copy link
Contributor

@donbowman
I agree this is considered bad practice, and probably should be removed, but I want to give you some context around why RDFLib has always printed the version of the library when imported.

The RDFLib project has been in constant and active development since 2001. It was originally written for Python 1.6.1, and in 2002 ported to Python 2.1. Back then, it was common for most python libraries to print their version to stdout when importing. It was actually considered best practice at that time. And while the rdflib code has been updated and tweaked to work on newer python versions over the last 20 years, a lot of the development practices in the codebase have not. One major reason for this, is because until fairly recently the rdflib codebase had been maintained by a small team consisting of remnants from the original developers, who still held onto the original development practices. Also important to note, the vast majority of contributors to RDFLib over the years are researchers, academics, ontologists, not software developers.

We, the new maintainers are slowly trying to fix issues like this as they pop up, but we need to be mindful of not changing too much too quickly. A lot of people have been using rdflib for 20 years, and expect it to operate in a specific way. For example, there is likely more than one textbook used in universities around the world which includes something like "type import rdflib in REPL, and you will see your currently installed RDFLib version printed in the output".

Having said that, I agree this is a good change, and should be removed before releasing the next version (actually probably should have been removed for v6.0.0, which included several other similar breaking changes).

@nicholascar nicholascar merged commit f9ac35b into RDFLib:master Sep 13, 2021
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.

remove logging from rdflib in production code
4 participants