search

MassBank / RMassBank

Playground for experiments on the official http://bioconductor.org/packages/devel/bioc/html/RMassBank.html
Other
12 stars 15 forks source link

Implemented log-wrapper #291

Closed pstahlhofen closed 2 years ago

pstahlhofen commented 3 years ago

This addresses #287

pstahlhofen commented 2 years ago

@tsufz @meowcat What do you think? If you like the current behaviour of the logger, I can extend its usage to other parts of RMassBank. Otherwise, I'd be happy about suggestions for improvements. To test this, reinstall RMassBank from the latest commit of this pr and run the mbWorkflow. If you want the logger to write to a file, set the logging_file in the settings.

tsufz commented 2 years ago

@pstahlhofen, I will check the code next week. I need to process some spectra anyway.

meowcat commented 2 years ago

I like it. However, could you change the log_XXX functions (which call the update_appender) to something like rmb_log_XXX? I don't think there's the need to "overwrite" the logger names, particularly when you export the functions. Or is there a rationale for that?

Also, I'm not sure that you should export the functions, but I'm not completely sure about this :)

meowcat commented 2 years ago

By the way, I found this https://github.com/daroczig/logger/blob/master/vignettes/r_packages.Rmd about logging from packages. Might be good to use this namespace thing.

pstahlhofen commented 2 years ago

@meowcat thanks for the hint. The namespace allowed me to drop the internal package variable and to remove the update_appender function. Also, I did. the renaming as you suggested. I think the functions should remain exported so users can apply them for debugging

pstahlhofen commented 2 years ago

@tsufz @meowcat @schymane @sneumann I will now merge the logger implementation as this pr has been open for almost two months now and it is blocking others from being merged. The reason I mentioned all of you here is because I replaced all calls to message in all scripts by calls to rmb_log_info and I would like you to use that function in the future instead of message. @tsufz I checked #292 and it seems like you can easily resolve the merge conflict in the DESCRIPTION which will arise now by keeping both changes. Note: So far, I only replaced calls to message to give us a test phase with the new logging system. If you like the behaviour, I'll make a second pr in the near future, replacing also calls to cat, warning etc. If something is odd, however, please let me know. I'm only merging into 'dev' so there's no harm done on BioConductor.