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

implement report() #278

Merged
merged 5 commits into from
Sep 28, 2021
Merged

implement report() #278

merged 5 commits into from
Sep 28, 2021

Conversation

mrcaseb
Copy link
Member

@mrcaseb mrcaseb commented Sep 16, 2021

closes #274

added the function report() that should help us debug problems by outputting version information

@mrcaseb
Copy link
Member Author

mrcaseb commented Sep 16, 2021

This also includes a nice 3 column implementation of dependencies @tanho63

nflfastR::report("nflfastR", TRUE)
#> -- System Info -----------------------------------------------------------------
#> * R version 4.1.1 (2021-08-10)
#> * OS: x86_64-w64-mingw32/x64 (64-bit)
#> -- Packages --------------------------------------------------------------------
#> * nflfastR (4.2.0.9006)
#> -- Dependencies ----------------------------------------------------------------
#> * cli        (3.0.1)     * codetools   (0.2-18)    * cpp11      (0.3.1) 
#> * crayon     (1.4.1)     * curl        (4.3.2)     * data.table (1.14.0)
#> * digest     (0.6.27)    * dplyr       (1.0.7)     * ellipsis   (0.3.2) 
#> * fansi      (0.5.0)     * fastrmodels (1.0.2)     * furrr      (0.2.3) 
#> * future     (1.22.1)    * generics    (0.1.0)     * globals    (0.14.0)
#> * glue       (1.4.2)     * janitor     (2.1.0)     * jsonlite   (1.7.2) 
#> * lattice    (0.20-44)   * lifecycle   (1.0.0)     * listenv    (0.8.0) 
#> * lubridate  (1.7.10)    * magrittr    (2.0.1)     * Matrix     (1.3-4) 
#> * mgcv       (1.8-36)    * nlme        (3.1-152)   * parallelly (1.28.1)
#> * pillar     (1.6.2)     * pkgconfig   (2.0.3)     * progressr  (0.8.0) 
#> * purrr      (0.3.4)     * R6          (2.5.1)     * Rcpp       (1.0.7) 
#> * rlang      (0.4.11)    * snakecase   (0.11.0)    * stringi    (1.7.4) 
#> * stringr    (1.4.0)     * tibble      (3.1.4)     * tidyr      (1.1.3) 
#> * tidyselect (1.1.1)     * utf8        (1.2.2)     * vctrs      (0.3.8) 
#> * xgboost    (1.4.1.1)   * codetools   (0.2-18)    * cpp11      (0.3.1)

Created on 2021-09-16 by the reprex package (v2.0.1)

Copy link
Member

@guga31bb guga31bb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use the new pipe we have to increment the R version requirement in DESCRIPTION, right? I would be inclined to not do this (I can't even test this bc I don't have this version of R, lol)

@mrcaseb
Copy link
Member Author

mrcaseb commented Sep 16, 2021

If we use the new pipe we have to increment the R version requirement in DESCRIPTION, right? I would be inclined to not do this (I can't even test this bc I don't have this version of R, lol)

Good catch! That was a mistake. Will fix

@guga31bb guga31bb linked an issue Sep 16, 2021 that may be closed by this pull request
@tanho63
Copy link
Member

tanho63 commented Sep 16, 2021

I'm not opposed to bringing in cli at nflreadr level if it makes more sense for this to live there, does it make more sense?

@guga31bb
Copy link
Member

Ok looks good to me!

@mrcaseb
Copy link
Member Author

mrcaseb commented Sep 16, 2021

I'm not opposed to bringing in cli at nflreadr level if it makes more sense for this to live there, does it make more sense?

I wanted to test the function with cli and found two special cases that need to be caught haha. Adding cli to nflreadr would also add glue and I'd like to keep it light so I would put report into nflfastR tbh

nflfastR::report("cli", TRUE)
#> -- System Info -----------------------------------------------------------------
#> * R version 4.1.1 (2021-08-10)
#> * OS: x86_64-w64-mingw32/x64 (64-bit)
#> -- Packages --------------------------------------------------------------------
#> * cli (3.0.1)
#> -- Dependencies ----------------------------------------------------------------
#> * glue (1.4.2)

@tanho63
Copy link
Member

tanho63 commented Sep 16, 2021

trouble to me is it becomes annoying to re-export a report function if we're debugging anything other than nflfastR?

@mrcaseb
Copy link
Member Author

mrcaseb commented Sep 17, 2021

trouble to me is it becomes annoying to re-export a report function if we're debugging anything other than nflfastR?

I am not sure what that means tbh. I assume most users that are asking us nflverse related questions will have installed nflfastR anyways. And if that's not the case they just need to install it an run the function. Sounds like a fair price for not adding another two deps to nflreadr. But that is not a strong opinion.

@mrcaseb
Copy link
Member Author

mrcaseb commented Sep 28, 2021

Going to merge this now. We can still move the function in another package if we feel like it is necessary. It's not a function that is a typical use case in any user script so moving it is not a big deal at all

@mrcaseb mrcaseb merged commit 310cd23 into master Sep 28, 2021
@mrcaseb mrcaseb deleted the report branch September 28, 2021 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants