-
Notifications
You must be signed in to change notification settings - Fork 235
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
Deprecate PEcAn.logger
in favor of log4r
?
#3223
Comments
In fact log4r offers much more used logs other than our |
I don't have a stong objection, but I also file this under the category of "if it's not broke, don't fix it". There's a number of places in PEcAn where the code is genuinely buggy and unreliable, and I'd personally prioritize those fixes over ones that are mostly aesthetic, especially since this swap could actually introduce new bugs as we learn how to use log4r. Also creates an additional external dependency, which could leave us in a pain point when this package breaks, is deprecated, is upgraded to new syntax, or is dropped by CRAN. All these things have happened to us many time with other packages. |
I agree with Mike. I have some ideas about ways we could improve logging around the edges*, but what we have basically works and any changes to it are low priority. *In particular: I think it may be possible to have the logger register itself as a condition handler so that it can handle built-in |
Might be more trouble than its worth at this point, but on the other hand, on a cursory look, it seems like
log4r
does everythingPEcAn.logger
does and that would mean one fewer packages to get on CRAN and maintain.https://github.com/johnmyleswhite/log4r
The text was updated successfully, but these errors were encountered: