Skip to content

Conversation

@m7pr
Copy link
Contributor

@m7pr m7pr commented Nov 4, 2024

Part of #216

Changes:

  • moved dependency extraction from get_code_dependency to eval_code
  • removed extract_code_graph
  • extended documentation of qenv with 1 new attributes: dependency, occurrence
  • merged side_effects and occurrence inside eval_code as they were previously joined in extract_code_graph anyway
  • created tests for qenv() |> eval_code |> get_code_attr("dependency")
  • changed extract_side_effects and extract_occurrence so they work on an element, and they don't use lapply

@m7pr m7pr requested a review from gogonzo November 5, 2024 11:54
@m7pr
Copy link
Contributor Author

m7pr commented Nov 5, 2024

Hey @gogonzo - if you have some spare time, this PR is ready for the first review.
It aims for transition of code_graph calculations from get_code_dependency into single calls in eval_code


pd <- utils::getParseData(current_call)
pd <- normalize_pd(pd)
call_pd <- extract_calls(pd)[[1]]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extract_calls while calls are already split?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extract_occurrence assume that pd has a specific order of rows, that gets changed when you apply extract_calls(pd) on pd.

I can move call_pd <- extract_calls(pd)[[1]] inside extract_occurrence. Or is it ok to keep in eval_code?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me ask another question - why to reorder pd?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extract_occurrence assumes object names (SYMBOL) appear after <- (LEFT_ASSIGN/ASSIGN).

extract_calls reorders pd in this way.

code <- "a<-1"
parsed_code <- parse(text = code, keep.source = TRUE)
pd <- utils::getParseData(parsed_code)
> pd
  line1 col1 line2 col2 id parent       token terminal text
7     1    1     1    4  7      0        expr    FALSE     
1     1    1     1    1  1      3      SYMBOL     TRUE    a
3     1    1     1    1  3      7        expr    FALSE     
2     1    2     1    3  2      7 LEFT_ASSIGN     TRUE   <-
4     1    4     1    4  4      5   NUM_CONST     TRUE    1
5     1    4     1    4  5      7        expr    FALSE     
> extract_calls(pd)
  line1 col1 line2 col2 id parent       token terminal text
7     1    1     1    4  7      0        expr    FALSE     
3     1    1     1    1  3      7        expr    FALSE     
2     1    2     1    3  2      7 LEFT_ASSIGN     TRUE   <-
5     1    4     1    4  5      7        expr    FALSE     
1     1    1     1    1  1      3      SYMBOL     TRUE    a
4     1    4     1    4  4      5   NUM_CONST     TRUE    1

If we don't want to use extract_calls we need to revisit and rewrite extract_occurrence. I am not sure we will invent rules to figure out dependencies for a non-sorted pd.

So right now this is a matter of whether we

  • use extract_calls on pd and before we apply pd inside extract_occurrence
  • or whether we put extract_calls inside extract_occurrence
  • or whether we rewrite extract_occurrence.

extract_occurrence is a pretty big beast : P

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can simplify extract_calls part so that it only reorders

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just played with what parts of extract_calls are needed so that extract_occurrence works and below are the needed parts:

# reordering basen on parent-children relation
parent_ids <- pd[pd$parent == 0 & (pd$token !e= "COMMENT" | grepl("@linksto", pd$text, fixed = TRUE)), "id"]
pd_order <- do.call(rbind, lapply(parent_ids, function(parent) rbind(pd[pd$id == parent, ], get_children(pd, parent))))
# filtering or edge cases
if (!is.null(pd_order) && !(nrow(pd_order) == 1 && call$token == "';'")) {
    # fixing assignment arrows
    pd_order <- fix_arrows(list(pd_order))[[1]]
    attr(current_code, "dependency") <- c(extract_side_effects(pd_order), extract_occurrence(pd_order))
}

So this is basically all of extract_calls without fix_shifted_comments that is skipped if there is only 1 call.

If extract_calls would be renamed to reorder_and_clean_calls then I suppose usage of this function is totally justified in here. We can always put this part in extract_occurrence but it's gonna be repeated in extract_calls and extract_occurrence

@m7pr
Copy link
Contributor Author

m7pr commented Nov 6, 2024

Hey @gogonzo I also needed to change the way we create teal.data::teal_data()
a0d4890b7816b614ba353be8db2175ccfb6c39ed
so that it's @code field corresponds to the changes made in here.

There is a bit code from teal.code for unexported functions. Should we export something in teal.code, so that it could be reused in teal.data?

}

attr(current_code, "id") <- sample.int(.Machine$integer.max, size = 1)
attr(current_code, "dependency") <- extract_dependency(current_call)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This loop looks nice now. extract_dependency is a good move 👍

@m7pr m7pr requested a review from gogonzo November 6, 2024 12:58
Copy link
Contributor

@gogonzo gogonzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good here 👍 Please see the comment below

@m7pr m7pr merged commit 6f03292 into 211_subset@main Nov 6, 2024
1 check passed
@m7pr m7pr deleted the 211_subset_simplify@main branch November 6, 2024 13:09
@github-actions github-actions bot locked and limited conversation to collaborators Nov 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants