Skip to content
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

code shouldn't be stored as character #181

Open
pawelru opened this issue Dec 4, 2023 · 5 comments
Open

code shouldn't be stored as character #181

pawelru opened this issue Dec 4, 2023 · 5 comments
Labels

Comments

@pawelru
Copy link
Contributor

pawelru commented Dec 4, 2023

In R, there are more appropriate structures to store the code but we keep storing this as a character.

This has some indirect negative consequences of unnecessary type conversions. Right now, if I call quenv() |> within({<code>}) then my <code> is converted to character and then back again to the code. Such behaviour is currently being adapted in teal.data (eval_code.teal_data_module) so if changed - please change there as well.

@m7pr
Copy link
Contributor

m7pr commented Dec 18, 2023

Hey @pawelru is this still relevant? Should we close?
Code needs to be stored as character so that we can include comments in the code. If you store the code as expression or language, then you can not have comments. Comments are needed as we include special tags in comments that allows us to bind specific code lines with objects that they have an influence on.

@pawelru
Copy link
Contributor Author

pawelru commented Dec 20, 2023

There are two aspects in your response

  1. comments
    I personally have some problems with understanding the requirement of supporting comments in the code. My understanding of code is everything what you evaluate - i.e. excluding comments. Comments are supplementary helper text lines living in the script alongside code. For me, script =/= code but it's more like (script) -parse()-> (code) and parse() strips off any comments:
r$> parse(text = "
    # aaa
    1 + 1
    ")
expression(1 + 1)

Comments are needed as we include special tags in comments that allows us to bind specific code lines with objects that they have an influence on.

I have a lot of doubts for a code dependency feature as a whole. First of all, it cannot be correctly done simply because of expressions with side effects. This would require some additional input such as comments. Here, comments looks to be a good way of providing this additional information but again, I really doubt that it's actually used as input code is usually quite lengthy and it's a lot of work.
Leaving all of those aside, let's go to the crux of the issue - assuming we accept code as character - should we continue keep it and use it as character? As an alternative, on init we can parse char to code (and store it in this form) and also build additional metadata (such as expr to objects associations).

@m7pr
Copy link
Contributor

m7pr commented Dec 20, 2023

I have a lot of doubts for a code dependency feature as a whole. First of all, it cannot be correctly done simply because of expressions with side effects.

What do you mean? All side-effects that can not be connected to objects will have a special tag in a comments in the line where the side-effect is created. For sure this is hard challenge to tackle, but this is only a data preparations part so I would not expect weird code being provided.

expression({some_side_effect_on_all_objects}) # @linksto object_name_1 object_name_2

with the above you can connect a line to an object.


As an alternative, on init we can parse char to code (and store it in this form) and also build additional metadata (such as expr to objects associations).

I don't think I understand this part

@kpagacz
Copy link
Contributor

kpagacz commented Nov 22, 2024

I am on the wagon of scrapping the whole code dependency feature. I don't think it adds much (less code in Show R Code, but that's really it) and is, as I recently found out (#233), incredibly brittle, in the sense that I could break it in a million ways without even trying.

@chlebowa
Copy link
Contributor

chlebowa commented Nov 22, 2024

@kpagacz
The feature was intended not as a mere convenience (less code in Swho R Code) but as a foundation for splitting and combining qenvs. It was not built exhaustively, which is why you get to complain adbout variable names, etc.
data.tables are actually a point of vulerability for teal.code as a whole because one can modify a data.table in a qenv without evaluating code in the qenv.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants