Closed n8downs closed 1 week ago
Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.
Thanks for integrating Codecov - We've got you covered :open_umbrella:
Thanks @n8downs. I totally get where you are coming from, and would be frustrated in your shoes as this change was not listed in the changelog.
Logging can be done in many different ways, and is taken personally by some (though not many) of our users; the philosophy of Brightway is definitely to allow people to do what they want.
I am merging this PR. I have been using loguru in recent development, and would appreciate your input on whether a simpler library like this, but one which still allows for other logging patterns would make sense to you. In particular, I would love some pointers on how we can help make it easy for you to use whatever logger you want while still producing nice and easy logs for the 90% of our users who don't even know that logging is a thing. One possibility would be to add logger
as in input argument with loguru
as a default, but I am open to other suggestions.
@jsvgoncalves FYI
Wow, thanks for the quick turnaround!
Happy to provide some thoughts on logging, but feel free to take them with a grain of salt coming from a javascript guy getting back into python.
Loguru looks to be pretty slick! They've got a recommendation for logging as a library: https://github.com/Delgan/loguru?tab=readme-ov-file#suitable-for-scripts-and-libraries
Alternatively, you could follow the general python guidelines for logging as a library: https://docs.python.org/3/howto/logging.html#configuring-logging-for-a-library
Then, to enable logs your users would need to
from loguru import logger
logger.enable('brightway')
# OR
import logging
logging.getLogger('brightway').addHandler(...)
Either way, I think a setup note for how to enable the brightway logs in your users' applications would suffice to make sure users can still get at important logged info while allowing more power users control over their standard out.
Thanks Nathan-
Disabling logs by default will work in some cases but not others. For example, in bw_simapro_csv we use multiple logs to provide both real-time and after the fact feedback to users, and these logs can't be disabled by default while keeping the library usable.
In these cases, I will try a combination of explicit documentation on configuration and removal, and following the expected loguru
conventions more closely.
FYI I would like to do the same with bw2calc
, and get feedback from you, before making a new release.
Yeah, I think as long as it can be disabled and if logging is an important avenue of feedback from the library, it's fine to leave it turned on by default.
Happy to give feedback on this update when it's ready.
Problem
After updating to
2.0.dev17
, we're suddenly seeing our LCA server logs transformed into JSON. While JSON logging is not in and of itself a bad thing, I'd prefer to have control over the format of our logs, rather than have them dictated as a side-effect of importing a dependency package. Prior to https://github.com/brightway-lca/brightway2-calc/commit/9b2ce5b2cdd97ae09da894b94346a3f24d5ea121, it looks like this logging init was optional.Before
After
I couldn't find a way to override this change via something like
json_logging.init_non_web(enable_json=False)
either before or after thebw2calc
import, so I'm open to workaround ideas.Solution
Remove the dependency on
json_logging
and the initialization of JSON logging. I'd argue that log format should be a decision that the user makes rather than one determined by this package.