gumyr / cq_warehouse

A cadquery parametric part collection
Apache License 2.0
107 stars 23 forks source link

Package should not configure logging #55

Closed sethfischer closed 1 year ago

sethfischer commented 2 years ago

Libraries should not configure logging. Rather logging configuration should be left to the application.

See Configuring Logging for a Library which recommends something like:

import logging
logger = logging.getLogger("cq_warehouse")

logger.info("Info...")

which will log to sys.stderr.

Or to only log if the application is using logging:

import logging
logger = logging.getLogger("cq_warehouse").addHandler(logging.NullHandler())

Currently extensions.py has:

logging.basicConfig(
    filename="cq_warehouse.log",
    encoding="utf-8",
    level=logging.DEBUG,
    format="%(asctime)s - %(levelname)s - [%(filename)s:%(lineno)s - %(funcName)20s() ] - %(message)s",
)

With logging.basicConfig as above cq_warehouse configures the root logger meaning that any application using cq_warehouse and logging, logs to file cq_warehouse.log. Including the application messages.

If cq_warehosue used logging.getLogger("cq_warehouse") the application would be able to use logging.basicConfig to configure logging as expected, including:

All messages sent to the cq_warehouse logger would propagate to the root logger which by default is configured to log to sys.stderr.


I discovered this as my application was using logging.basicConfig to configure logging. After importing cq_warehouse.extensions my application no longer logged to the console, instead my application log messages were written to file cq_warehouse.log.

gumyr commented 2 years ago

Thanks! Nice to see someone else using logging and I don't want cq_warehouse to interfere so this will have to be fixed.

gumyr commented 1 year ago

I've experimented with this quite a bit and have been unable to come up with a way to set the format of cq_warehouse logs while fixing this problem and using the Null handler. Unless you have a better suggestion, I'll put a note into the documentation some where for the user to set the format and go with the following (as shown in this little prototype) if this is going to be okay:

# cq_warehouse.py
import logging

logging.getLogger("cq_warehouse").addHandler(logging.NullHandler())
logger = logging.getLogger("cq_warehouse")

def cq_warehouse_extension():
    logger.info("cq_warehouse log")
# cq_app.py
import logging
import cq_warehouse

def main():
    logging.basicConfig(
        filename="myapp.log",
        level=logging.INFO,
        format="%(name)s-%(levelname)s %(asctime)s - [%(filename)s:%(lineno)s - %(funcName)20s() ] - %(message)s",
    )

    logging.info("1st app log")
    cq_warehouse.cq_warehouse_extension()
    logging.info("2nd app log")

if __name__ == "__main__":
    main()

Which generates the following logs:

root-INFO 2022-08-04 10:28:39,011 - [cq_app.py:13 -                 main() ] - 1st app log
cq_warehouse-INFO 2022-08-04 10:28:39,011 - [cq_warehouse.py:9 - cq_warehouse_extension() ] - cq_warehouse log
root-INFO 2022-08-04 10:28:39,011 - [cq_app.py:15 -                 main() ] - 2nd app log

I guess the only important thing in the formatting is to use %(name)s as this differentiates between root (or application) logs and cq_warehouse logs.

Following python best practices and configuring the NullHandler() means that if the user doesn't configure logging, all logs (including WARNING) are discarded.

Tagging @afshawnlotfi as he's also identified this problem.

gumyr commented 1 year ago

Closed. cq_warehouse doesn't interfere with user's logging configuration.