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

Add S3-method based formatting #22

Closed
wants to merge 1 commit into from
Closed

Conversation

bobjansen
Copy link

This would allow definition of formatting methods per S3-class. As a proof of concept, I've added pander to do this as done in your SO answer.

@daroczig
Copy link
Owner

@bobjansen -- so sorry for the long delay with my reply here :(

So I was not sure how to do this properly and played around a little bit trying to figure out how to be able to pass parameters to pander and keep the API similar to the other formatters. After all, I think we can limit a log call to one object at a time so thus we can pass optional params to pander down ... does it make sense?

This way we do not need the extra S3 step as pander is already an S3 method.

Please see the just pushed commit and let me know what you think.

On the other hand, do you want to pass multiple objects to the same log call and would rather skip the option to pass optional params to pander or whatever the S3 method dispatches?

@codecov-io
Copy link

codecov-io commented Jul 25, 2019

Codecov Report

Merging #22 into master will decrease coverage by 0.39%.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master     #22     +/-   ##
========================================
- Coverage   72.89%   72.5%   -0.4%     
========================================
  Files          11      11             
  Lines         369     371      +2     
========================================
  Hits          269     269             
- Misses        100     102      +2
Impacted Files Coverage Δ
R/formatters.R 80.85% <0%> (-3.6%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c336a37...e4a8aac. Read the comment docs.

@daroczig
Copy link
Owner

Example run:

> log_formatter(formatter_pander)
> log_info('42')
INFO [2019-07-25 09:31:34] 42
> log_info(40 + 2)
INFO [2019-07-25 09:31:37] _42_
> log_info(head(iris))
INFO [2019-07-25 09:31:41] 
INFO [2019-07-25 09:31:41] -------------------------------------------------------------------
INFO [2019-07-25 09:31:41]  Sepal.Length   Sepal.Width   Petal.Length   Petal.Width   Species 
INFO [2019-07-25 09:31:41] -------------- ------------- -------------- ------------- ---------
INFO [2019-07-25 09:31:41]      5.1            3.5           1.4            0.2       setosa  
INFO [2019-07-25 09:31:41] 
INFO [2019-07-25 09:31:41]      4.9             3            1.4            0.2       setosa  
INFO [2019-07-25 09:31:41] 
INFO [2019-07-25 09:31:41]      4.7            3.2           1.3            0.2       setosa  
INFO [2019-07-25 09:31:41] 
INFO [2019-07-25 09:31:41]      4.6            3.1           1.5            0.2       setosa  
INFO [2019-07-25 09:31:41] 
INFO [2019-07-25 09:31:41]       5             3.6           1.4            0.2       setosa  
INFO [2019-07-25 09:31:41] 
INFO [2019-07-25 09:31:41]      5.4            3.9           1.7            0.4       setosa  
INFO [2019-07-25 09:31:41] -------------------------------------------------------------------
INFO [2019-07-25 09:31:41] 
> log_info(head(iris), style = 'simple')
INFO [2019-07-25 09:31:43] 
INFO [2019-07-25 09:31:43] 
INFO [2019-07-25 09:31:43]  Sepal.Length   Sepal.Width   Petal.Length   Petal.Width   Species 
INFO [2019-07-25 09:31:43] -------------- ------------- -------------- ------------- ---------
INFO [2019-07-25 09:31:43]      5.1            3.5           1.4            0.2       setosa  
INFO [2019-07-25 09:31:43]      4.9             3            1.4            0.2       setosa  
INFO [2019-07-25 09:31:43]      4.7            3.2           1.3            0.2       setosa  
INFO [2019-07-25 09:31:43]      4.6            3.1           1.5            0.2       setosa  
INFO [2019-07-25 09:31:43]       5             3.6           1.4            0.2       setosa  
INFO [2019-07-25 09:31:43]      5.4            3.9           1.7            0.4       setosa  
INFO [2019-07-25 09:31:43] 
> log_info(lm(hp ~ wt, mtcars))
INFO [2019-07-25 09:31:46] 
INFO [2019-07-25 09:31:46] ----------------------------------------------------------------
INFO [2019-07-25 09:31:46]      &nbsp;        Estimate   Std. Error   t value    Pr(>|t|)  
INFO [2019-07-25 09:31:46] ----------------- ---------- ------------ ---------- -----------
INFO [2019-07-25 09:31:46]  **(Intercept)**    -1.821      32.32      -0.05633    0.9555   
INFO [2019-07-25 09:31:46] 
INFO [2019-07-25 09:31:46]      **wt**         46.16       9.625       4.796     4.146e-05 
INFO [2019-07-25 09:31:46] ----------------------------------------------------------------
INFO [2019-07-25 09:31:46] 
INFO [2019-07-25 09:31:46] Table: Fitting linear model: hp ~ wt
INFO [2019-07-25 09:31:46] 

@bobjansen
Copy link
Author

Apologies for taking so long, this looks good! I've rebased the commits into one.

To your question: I don't think I would want to log multiple objects at once.

@bobjansen bobjansen changed the title [WIP] Add S3-method based formatting Add S3-method based formatting Sep 24, 2019
@daroczig daroczig closed this in 50db8dc Oct 18, 2019
@bobjansen
Copy link
Author

Thanks for this! Do you have any plans for an update of the package on CRAN?

@daroczig
Copy link
Owner

Yeah, I'm waiting for my botor pkg to get accepted there so that I can merge #35 and then I'll push an update to CRAN -- no ETA though, I've started that process at the very beginning of Sept 🤷‍♂️

@daroczig
Copy link
Owner

daroczig commented Dec 4, 2019

See also #40

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

Successfully merging this pull request may close these issues.

None yet

3 participants