-
-
Notifications
You must be signed in to change notification settings - Fork 7
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
[Feature Request]: Extend within
to accept a custom parameter that links all lines to a dataname
#200
Comments
Note that |
Some context: this idea was inspired by working with the python app in teal.gallery, where the code setting the environment is not traced back to the dataname. Hence, an This is an idea/discussion on how to bring it to
If this avoids executing string as code I think it's an acceptable trade-off
Valid, we could further minimize this by adding a dot as a prefix to avoid names clashing, similar to what tidyverse does.
|
An alternative solution might be a validation of the completeness of the code. So that when (pseudo-code below) is not empty, it returns all the code.
That is, fallback to the full code on the code parser blindspots |
I'm not a fan of this approach. The point of the |
We tried to remove Tagging code in
This would be an effective approach 👍 I don't quite like the mixed formal names, though (dot vs no dot). I'm on the fence on this. |
I don't think it should become obsolete. Especially if we have users executing code from other scripts or blocks of texts. Having said that, I don't like using it inline as it obfuscates expressions inside a very long string. Among the cons are:
|
I'm not a huge fan of adding new non-standard parameters to I'll think a bit more about it. |
This is exactly why we decided to use comments tag instead of extra argument. Imagine this # now
data |>
within({
open_connection() # @linksto df1 df2
df1 <- get_data("df1")
df2 <- get_data("df2")
close_connection() # @linksto df1 df2
})
# possibly
data |>
within(open_connection(), .linksto = c("df1", "df2")) |>
within({
df1 <- get_data("df1")
df2 <- get_data("df2")
}) |>
within(close_connection(), .linksto = c("df1", "df2")) I think
Maybe, we can have both However, there is fundamental question. We need to asses the cost of maintaining code-subsetting and decide whether this feature is worth efforts. |
AFAIK my_within <- \(data, expr, ...) {
expr <- substitute(expr) # first line of within.qenv
expr
}
my_within(iris, {
1+
2 # a comment
})
#> {
#> 1 + 2
#> }
Agree 👍
As @chlebowa said, this is not great 😁 and I agree. My only grip is that using string code is the only alternative for the code parser limitations. # Now
data |>
eval_code(
"
open_connection() # @linksto df1 df2
df1 <- read_lines(1L)
df2 <- read_lines()
close_connection() # @linksto df1 df2
"
)
Totally. This is not a trivial feature as it needs to be tightly integrated with code parser. |
Looking at the commented code I wonder if we could give a hint to the code parser via a "side effect" function. (sorry if this has already been discussed) # Possibly (alt to subsetting)
teal_data() |>
within({
open_connection() %@linksto% list(df1, df2)
df1 <- get_data("df1")
df2 <- get_data("df2")
close_connection() %>% teal.code::links_to(df1, df2)
})
`%@linksto%` <- \(x, ...) x # or %links_to% or just %hint%
1 + 1 %@linksto% list(df_not_exist, df2)
#> [1] 2
links_to <- `%@linksto%`
1 + 1 |> links_to(df_variable_doesnt_exist)
#> [1] 2 |
Yup,
@averissimo Good luck with writing parser which handles this expression :D If we introduce I imagine such thing, so that
|
In
Then,
The code is essentially collapsed for all intents and purposes, we just temporarily store it separated. What if we do the collapsing
I think this is doable.
Now this is is a can of worms... 😫 |
Feature description
Extend
within.qenv(data, expr, links_to = NULL, ...)
so thatlinks_to
parameter will hint the code parserThe goal is that all the code in expr is hinted to one dataname (?or more?).
The
get_code
below would output the same code.Created on 2024-02-20 with reprex v2.0.2
Code of Conduct
Contribution Guidelines
Security Policy
The text was updated successfully, but these errors were encountered: