get_code to return concatenated code string and removed format_expression

[kartikeyakirar]

this fixes #173

In this pull request, I've made updates to both the get_code and replace_code methods. Now, when the deparse arg is set to TRUE, these methods will return a concatenated string. Specifically, the get_code method will return character(0) when used with new_qenv(), provided no code has been set. As for replace_code, it will first split the input string using "\n", then replace the last string in this sequence. Finally, it concatenates these strings again before replacing the code.

Removed format_expression function.

Example

q <- new_qenv()
get_code(q) # return character(0)
replace_code(q, "i <- iris")  # @code return character(0)

qq <- within(q, i <- iris)
qq <- within(qq, m <- mtcars)
get_code(qq) # return "i <- iris\nm <- mtcars"

replacement <- "i <- mtcars"
qr <- replace_code(qq, replacement)
get_code(qr) #return "i <- iris\ni <- mtcars"

Also review following modules for PR for changes.

• teal get_code to return concatenated code string  teal#976
• teal.data get_code to return concatenated code string and removed format_expression teal.data#206
• teal.modules.general get_code to return concatenated code string teal.modules.general#615
• teal.modules.clinical get_code to return concatenated code string teal.modules.clinical#898
• teal.goshawk get_code to return concatenated code string teal.goshawk#250
• teal.osprey get_code to return concatenated code string teal.osprey#244

[vedhav]

I just have one comment regarding the changes for this issue. Other than that, I think everything looks fine to me. It's good that this issue is not a breaking change, so I believe updating just the NEWS.md is sufficient.

Could you please change the target branch to main instead of refactor in this and all the related child PRs? I think this issue is independent of the refactor changes, and we can merge it before or after the refactor without any issues.

[vedhav]

I'm having second thoughts about removing format_expression. It's used in multiple places. I think it can stay.

[kartikeyakirar]

I had a similar thought previously, but following a conversation with @chlebowa it was clarified that the function simply uses paste with a designated collapse parameter after processing with lang2calls. The format_expression function originally had a more complex design but it was simplified into a simple one-liner in a past refactoring phase. Currently, it's essentially redundant.

[chlebowa]

Correct. We don't want to have too simple wrappers. Check out the use of lapply(x, [[, y) throughout teal.slice. That used to be a function but we decided to use base calls.

On top of that, soon we will be down to just a few uses of format_expression anyway.

[m7pr]

format_expression sounds like a wrapper for paste, where you overwrite default collapse param :)

[averissimo]

Did you guys consider having lang2calls return the paste(...)?

It seems that it is only used this way in the package (other than in tests)

ps. and rename it to lang2str or lang2character?

[kartikeyakirar]

there is recursion in lang2calls and implementing the paste function with collapse directly inside lang2calls can lead to unintended results due to concatenation of results from each recursive call.

[m7pr]

@vedhav

Could you please change the target branch to main instead of refactor in this and all the related child PRs? I think this issue is independent of the refactor changes, and we can merge it before or after the refactor without any issues.

We also have get_code.teal_data that exists only on @refactor in teal.data so I would keep this aligned with @refactor branch.

[m7pr]

@kartikeyakirar why it only shortens the length for deparse = TRUE? For deparse=FALSE it returns an expression, but it could return an expression of length 1 as well as it returns pasted code into length 1 right?

[kartikeyakirar]

why it only shortens the length for deparse = TRUE? For deparse=FALSE it returns an expression, but it could return an expression of length 1 as well as it returns pasted code into length 1 right?

For deparse=FALSE it returns an expression not necessarily of length 1, The choice not to collapse the expressions into a single expression (even if possible) due to maintain integrity of the code. Collapsing everything into a single expression could alter the behavior or interpretation of the code when it's later evaluated.

Here expression is of length 3

q1 <- new_qenv(env = list2env(list(a = 1)), code = quote(a <- 1))
q2 <- eval_code(q1, code = quote(b <- a))
q3 <- eval_code(q2, code = quote(d <- 2))
length(get_code(q3)) # 1
length(get_code(q3, deparse = FALSE)) # 3

even if you collpase it before converting it into expression result is same

expression <- parse(text = get_code(q3), keep.source = TRUE)
length(expression) # 3

[m7pr]

@kartikeyakirar yeah, because you need to paste code with { and } to make it one call, hence an expression of length 1

> code <- c('a<-1', 'b<-2', 'c<-2')
> length(parse(text = code))
[1] 3
> length(parse(text = paste(code, collapse = '\n')))
[1] 3
> length(parse(text = paste(c('{', code, '}'), collapse = '\n')))
[1] 1

[kartikeyakirar]

because you need to paste code with { and } to make it one call,

yes. It won't alter the interpretation of the code when it's later evaluated. Why didn't I think of this! I will update test and make change in teal as well. thanks

[vedhav]

I don't have any other comments, Good job in reflecting the change in all repos. LGTM