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

Deprecate PEcAn.logger in favor of log4r? #3223

Open
Aariq opened this issue Sep 21, 2023 · 3 comments
Open

Deprecate PEcAn.logger in favor of log4r? #3223

Aariq opened this issue Sep 21, 2023 · 3 comments

Comments

@Aariq
Copy link
Collaborator

Aariq commented Sep 21, 2023

Might be more trouble than its worth at this point, but on the other hand, on a cursory look, it seems like log4r does everything PEcAn.logger does and that would mean one fewer packages to get on CRAN and maintain.

https://github.com/johnmyleswhite/log4r

@Sweetdevil144
Copy link
Contributor

Sweetdevil144 commented Apr 3, 2024

In fact log4r offers much more used logs other than our PEcAn.logger offers currently.

@mdietze
Copy link
Member

mdietze commented Apr 3, 2024

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.

@infotroph
Copy link
Member

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 message/warning/stop calls without the package that generates those messages needing to import anything. Slapping together a demo version of that has been low on my list for several years now...

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

No branches or pull requests

4 participants