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

Change logging from info to debug #163

Merged
merged 1 commit into from
Nov 15, 2022
Merged

Change logging from info to debug #163

merged 1 commit into from
Nov 15, 2022

Conversation

nvllsvm
Copy link
Contributor

@nvllsvm nvllsvm commented Nov 3, 2022

What
Change the the database-open log from info-level to debug-level

Why
It's an implementation detail that one does not need to care about while using this library.

@mpenkov
Copy link
Collaborator

mpenkov commented Nov 4, 2022

Dunno, I think your changes are OK, but I also think it's fine the way it is, because:

  1. This log statement gets executed rarely
  2. Whether or not people care about this statement is really up to them
  3. If they really don't care, then they can set the logging level to something else when configuring the logging library

@piskvorky thoughts?

@piskvorky
Copy link
Owner

piskvorky commented Nov 4, 2022

I'd keep it as-is, mostly on account of @mpenkov 's point 3). That's the reason why we use logging, so that people can configure the threshold the way they like.

While it's true there is some subjective arbitrariness in what is considered DEBUG (for developers) vs INFO (generally useful info but nothing vital), IMO "opening a database" falls squarely into INFO.

@nvllsvm why does it bother you?

@nvllsvm
Copy link
Contributor Author

nvllsvm commented Nov 14, 2022

Let me start by saying that I understand I'm being fairly pedantic about not liking the default of an info-level log entry. Thank you for discussing with me.

why does it bother you?

It's unexpected to have an otherwise unopinionated library output info-level statements by default. Libraries like the stdlib's sqlite3 and the popular pycopg do not output anything info-level when connecting to their respective databases. Tangentially, neither httpx not requests output info-level when making an HTTP request. These libraries delegate that responsibility to the developer using them.

For example, the only log entry output by this dict is from sqlitedict

import logging
import sqlite3

import psycopg
import requests
import sqlitedict

logging.basicConfig(level=logging.INFO)

sqlite3.connect('test.sqlite')
psycopg.connect('postgresql://somedb')
requests.get('https://github.com')
sqlitedict.SqliteDict('test.sqlitedict')
INFO:sqlitedict:opening Sqlite table 'unnamed' in 'test.sqlitedict'

That log entry is unnecessary noise in log output - especially when the output is shared with someone unfamiliar with sqlitedict or when multiple files are being used.

I know I can manually change the level of sqlitedict's logger, but then that becomes noise in the code - which results in a very slight increase in complexity for someone not familiar with sqlitedict (my team).

I also use this library in multiple, self-contained scripts so having to disable the library's opinion in each one is just one more thing I have to do.

@mpenkov
Copy link
Collaborator

mpenkov commented Nov 15, 2022

That log entry is unnecessary noise in log output - especially when the output is shared with someone unfamiliar with sqlitedict or when multiple files are being used.

OK, that's a reasonable point.

@mpenkov mpenkov merged commit 98a7991 into piskvorky:master Nov 15, 2022
@mpenkov
Copy link
Collaborator

mpenkov commented Nov 15, 2022

Merged, thank you for your effort and explanation.

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