Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Adds
within
andeval_code
toteal_data_module
#983Adds
within
andeval_code
toteal_data_module
#983Changes from 25 commits
03d5b7e
dc6739f
293f25a
3a8579d
c04eb56
2992271
f65a389
d8a095e
417c2ee
9605f05
fa6ef28
441652d
8bf8aaa
51dcadf
da3c100
c15c106
c2f708a
34db6c6
ef1877c
a861bec
e31ae85
3b9da92
71cfbf9
fc10479
af76edc
d9fdbb9
c012daf
f661743
b1625ba
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The body is a copy from
teal.code:::within.qenv
, it just prepares the arguments to calleval_code()
(data
is unchanged)So in practice it could be replaced with code below (reducing duplicated code accross
{teal.*}
):Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getS3method("within", "qenv", envir = as.environment(getNamespace("teal.code")))
would be a better choice.teal.code
must be installed whenteal
is.within
only processes the expression and callseval_code
. In this context this is a valid simplification. I'm not sure, though 🤔There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's better! Is the
envir
parameter required? (as you said,teal.code
is imported and it seems to find the within)@vedhav brough a smilar concern, I'm on the fence as I prefer to minimize repetitive code, but I don't think this solution is "best practice" (calling
within.qenv
with a different data object seems like a red flag).I would prefer if there was
within.default
(non-exported) or.generic_within
in{teal.code}
that was the backbone of both of those calls and still allow for deviations in the future ofwithin.qenv
that wouldn't affectwithin.teal_data_module
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't but just in case there is a different
within.qenv
defined somewhere...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
within
generic is abase
function so defining adefault
that suits our particular classes would cause a mess.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, so a
.generic_within
function would be best if we wanted to go this route.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the
.default
was a suggestion if it didn't interfere withbase::within
(by not exporting it, but even then it might be "dangerous")There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, but S3 methods must be exported 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gogonzo would you like to pitch in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One interesting pattern that I have found when working on the
verdepcheck
is to make a simple (i.e. non S3 / non S4 etc.) (implementation) function for each method and these methods are essentially a one-liner calling those functions. Please find an example in R6 context: https://github.com/r-lib/pkgdepends/blob/main/R/pkg-plan.R - all public methods are calling private (implementation) functions.Then you can re-use those implementation functions from multiple classes but there are some obvious drawbacks though such as number of objects in a package, documentation, maintenance, debugging etc.
This however is not addressing an exporting / importing issue mentioned here 😈
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Check warning on line 221 in tests/testthat/test-module_teal_with_splash.R
GitHub Actions / SuperLinter 🦸♀️ / Lint R code 🧶