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
Introduce the new DDL #957
Introduce the new DDL #957
Changes from 16 commits
c9887c9
e7a3526
e9df325
c2e84eb
a8f85ca
dc3d7e3
08c7214
221976a
55a5a80
51b3f79
e43c76a
f0c1cc6
df885f3
a324e21
3ac84f9
e11acd7
0bb2e70
9244d33
d6b8d2f
0ce0eb5
71c1501
0c52e1a
9b7de0f
640c27b
7a451d3
82fb25a
cfb27bc
c3adb74
147c02f
cd292ea
e80c8ce
724dcdb
a98682d
507fbc3
0f7accd
0836745
e511517
bbaff1f
2a38182
971c7d5
72abf29
457ae2c
9d63d40
f84ad0b
7d5ccc3
6570474
8a0675f
422efa6
8e0cc87
2c928b2
54f05ba
0b12f7b
f00e181
019f26c
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.
rename needed? Proposition:
env
andenv_mask
This is so specific to the
shiny
and if you do https://github.com/insightsengineering/teal/pull/957/files#r1381599897 then there will be no link to the shiny at all. I feel this functionality is of general use.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.
Good proposition,
env
andenv_mask
make sense as it is related directly tosubstitute
.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.
Yet another reason to move it to teal.code is that here you are coupling here to the class implementation (internal to teal.code) and not using public package API.
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.
Sounds good tbh
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.
Could be used in this way
app code
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.
Exactly, I think the same
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.
my preference would be exporting
delayed_data
as it might be more helpful to understand the class and concept itself to user.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.
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.
I have the same opinion. Besides,
delayed_data
might have much wider applicationThere 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.
I wasn't the fan of
teal_transform
which is nowdelayed_data
. But I guess if this solves the purpose and has the most benefits I think we should expose. From the other hand DDL sounds like a very specific use-case, so I would be fine with this as wellThere 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.
I don't think exposing makes it easier to understand as there is
server_formals
,extra_args
,server_args
,extra_formals
. If app developer understand that much of R and Shiny, would he be able to craft his own tool for his needs, instead of using teal? Even though I vote to exposeThere 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.
extra_args
andextra_formals
are just helpful to print relevant error message. There areserver
formals and...
which need to be matched. We need to throw meaningful message to alert that they don't match ;]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.
I think dark smog clouds has overcome my brain today :)
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.
why we are blocking usage of
...
in the providedserver
function?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.
I don't clearly see why one would use it but on the other hand I also don't see why one can't do this
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.
I will include this. I was speed-coding to give a working version to the discussion and I skipped
...
to not loose so much time then.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.
are you sure?
if yes - is this the only place where we use this function?
if yes - shall we remove it?
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, too early for this. I was testing something and pushed by mistake. Reverting.
FYI we will remove
resolve_delay
fromui
functions anyway soon.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.
Cool. Yes - I am expecting this to be removed but was surprised this happened so fast :P