search

ryapric / loggit

Modern Logging for the R Ecosystem
Other
37 stars 2 forks source link

`message()` doesn't capture all of `...` args, only the first #12

Closed brianwdavis closed 4 years ago

brianwdavis commented 4 years ago

I'm in the process of changing over from handmade log files to a uniform automated system, and so far I like loggit. However, a lot of my old scripts have lines like message("The number of rows written to db was: ", nrow(table)). However, the version that you implement for masking the base message ignores the multiple arguments and returns output like this:

> message("The number of rows: ", nrow(iris))
{"timestamp": "2020-05-25T16:50:48-0400", "log_lvl": "INFO", "log_msg": "The number of rows: "}
The number of rows: 150

The output to the console is correct! But the line written to the log only captures the first arg. It looks like this is an intentional design choice, because the source calls loggit with args[[1]] instead of something like paste(args, collapse = " ").

Is this changeable, or should I be adjusting my usage to match this behavior?

brianwdavis commented 4 years ago

I decided that having structure logs was worth the effort of structuring them, so I replaced the above example with loggit("INFO", "iris", rows = nrow(iris)) etc.

I remain curious about why the args[[1]] design decision was made, but it's no longer an issue.

ryapric commented 4 years ago

@brianwdavis Wooow, great catch! Actually not a design choice, explicitly: the big update recently focused on the ndjson backend rewrite, so I didn't touch any of the handler masks (their tests still passed!). I'm sure when I first wrote them, I'd Googled "how to work with dots in R function" or something and then just copy-pasted :)

Got it patched, and got the tests added. Changes are live on the master branch now. Haven't sent to CRAN yet, but you can grab v2.0.1 from Github now.

I see you just commented a change in your approach, as I was pushing the changes. I actually like that approach better! But you still raise a valid point, thank you very much for reaching out and letting me know!

For other readers: if you're not able to grab the GH release for some reason, then yes, you can get around this by pasting your own message/warning/stop content as a single string for now, until it hits CRAN.