Update vars_funs interface in the R package to match the Python package
#34
Conversation
… test it out" This reverts commit 9cc256b.
Co-authored-by: Dan Snow <[email protected]>
jeancochrane
force-pushed
the
jeancochrane/strip-out-unused-vars-funs-from-R-package
branch
from
3d58789 to
8e3b5ba
Compare
There was a problem hiding this comment.
Looks good @jeancochrane 👍 IMO we should just cut a new major version of this package once all the Python functions are merged and R functions updated.
There was a problem hiding this comment.
suggestion: IMO we can also remove the class_dict dataset that feeds this function, since it's now just a dbt seed that lives in data-architecture. I don't think it's used in any other major repos.
There was a problem hiding this comment.
Cool, done! I opened ccao-data/data-architecture#667 and ccao-data/ptaxsim#45 to fix the only references I could find outside this repo.
R/vars_funs.R
Outdated
| if (!is.null(type)) { | ||
| warning("'type' is deprecated. Use 'output_type' instead.", call. = FALSE) | ||
| output_type <- type | ||
| } | ||
|
|
||
| if (!is.null(dict)) { | ||
| warning("'dict' is deprecated. Use 'dictionary' instead.", call. = FALSE) | ||
| dictionary <- dict | ||
| } |
There was a problem hiding this comment.
suggestion (non-blocking): I recognize this is good practice, but I wonder about the utility of including lifecycle code like this in a strictly internal package. My preference would be to just release a new major version with the breaking changes included. Ultimately I defer to you @jeancochrane, as my view here isn't super strongly held.
There was a problem hiding this comment.
Makes sense, I stripped out this logic in 3d94d7e.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #34 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 6 6
Lines 382 377 -5
=========================================
- Hits 382 377 -5 ☔ View full report in Codecov by Sentry. |
jeancochrane
deleted the
jeancochrane/strip-out-unused-vars-funs-from-R-package
branch
This PR branches off of #32 to bring the R package up to parity with the changes that PR makes to the Python package. In particular:
vars_check_classfunctiontypeanddictparams invars_renameandvars_recode, and replace them with the new params{code|output}_typeanddictionarySince no code is currently using the
vars_check_classfunction, we can safely remove it. Code is still usingtypeanddict, however, so we implement deprecation with fallback so that that code will still work with the latest version of the package.Note that there is more code that we could strip out of the R package, but I'm restricting the scope of this PR to only the changes that are necessary to create parity with the Python package as of #32.