-
Notifications
You must be signed in to change notification settings - Fork 12
/
index.Rmd
231 lines (165 loc) · 8.33 KB
/
index.Rmd
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
---
title: "Automate code refactoring with {xmlparsedata} and {brio}"
date: '2024-05-15'
slug: refactoring-xml
output: hugodown::hugo_document
tags:
- xml
- refactoring
---
Once again a post [praising XML](/2022/04/08/xml-xpath/). :innocent:
These are notes from a quite particular use case: what if you want to replace the usage of a function with another one in many scripts, without manual edits and without touching lines that do not contain a call to replace?
The real life example that inspired this post is the replacement of all calls to `expect_that(..., equals(...))`, like `expect_that(a, equals(1))`, in igraph tests with `expect_equal()`.
If you're a newer package developer who grew up with testthat's third edition, you've probably never heard of that [cutesy old-school testing style](https://testthat.r-lib.org/reference/oldskool.html). :wink:
## Why automate? Where I subjectively justify my choice
As brilliantly explained by [XKCD 1205](https://xkcd.com/1205/), automation is not necessary worth the time.
In the case that motivated this post, automation was worth it because there were many test files, and because being able to regenerate all edits meant we can recreate the changes after merging other edits to the main branch, without any conflict.
## Parse the code to XML, detect problematic calls
For any `path`, we detect function calls to `expect_that()`.
The code is parsed using the `parse()` function, digested into XML with [{xmlparsedata}](https://github.com/r-lib/xmlparsedata).
```{r, eval=FALSE}
xml <- path |>
parse(keep.source = TRUE) |>
xmlparsedata::xml_parse_data(pretty = TRUE) |>
xml2::read_xml()
deprecated <- xml2::xml_find_all(
xml,
".//SYMBOL_FUNCTION_CALL[text()='expect_that']"
)
```
The `deprecated` object contains all the nodes we need to amend.
For info, here's how code parsed to XML looks like (yes, it is big despite representing two short lines of code):
```{r}
parse(text = "1+1\nsum(c(2,2))", keep.source = TRUE) |>
xmlparsedata::xml_parse_data(pretty = TRUE) |>
cat()
```
## Fix problematic calls in the XML representation
The `treat_deprecated()` function below tries to find a call to `equals()` inside the `expect_equal()`, since we only fix the calls to `expect_that()` that contain `equals()`.
We return early for these other cutesy expectations, with a warning so we can go look at the scripts and get an idea of what the calls are.
They will have to be fixed with another script, or manually, depending on how many of them there are.
For the calls to `expect_that()` that contain a call to `equals()`, we
- replace `expect_that()` with `expect_equal()`
- extract the text inside `equals()` to put it directly as second argument of `expect_equal()`.
Thus `expect_that(a, equals(1))` becomes `expect_equals(a, 1)`.
```{r, eval=FALSE}
treat_deprecated <- function(xml, path) {
siblings <- xml2::xml_parent(xml) |> xml2::xml_siblings()
equal <- siblings[grepl("equals\\(", xml2::xml_text(siblings))]
if (length(equal) == 0) {
cli::cli_alert_warning("WARNING AT {path}.")
return()
}
xml2::xml_text(xml) <- "expect_equal"
text <- xml2::xml_contents(equal)[[3]] |> xml2::xml_text()
xml2::xml_remove(xml2::xml_contents(equal))
xml2::xml_text(equal) <- text
}
```
## Serialize XML to character, write back
We only modify the lines of the script that need to be modified, as it will avoid spurious changes but also avoid figuring out how to serialize the whole HTML.
For each call that was edited in XML, we identify the corresponding start and end lines in the original script.
Below is again an example of parsing just to show that each expression has attributes called line1 and line2, the start and end lines.
```{r}
parse(text = "1+1\n2+2", keep.source = TRUE) |>
xmlparsedata::xml_parse_data(pretty = TRUE) |>
xml2::read_xml()
```
There are two cases:
- the start and end line is the same. We replace the corresponding line with the text of the grand-parent node.
- the start and end lines are different. We loop over them, for each of them we replace the corresponding line with the text of parent and uncles/aunts nodes.
The choice of parents/siblings might seem a bit arbitrary.
I made it work by putting a `browser()` in my code and figuring out what level of ancestry I had to deal with thanks to random tries.
I do not have a particularly good mental model of R code as XML. :wink:
For some reason I wrote for loops in the code below, probably because that's what made sense to me at the time. :shrug:
```{r, eval=FALSE}
lines <- brio::read_lines(path)
# ...
# code identifying deprecated calls
# ...
purrr::walk(deprecated, treat_deprecated, path = path)
for (deprecated_call in deprecated) {
parent <- xml2::xml_parent(xml2::xml_parent(deprecated_call))
line1 <- as.numeric(xml2::xml_attr(parent, "line1"))
line2 <- as.numeric(xml2::xml_attr(parent, "line2"))
if (line1 == line2) {
lines[line1] <- xml2::xml_text(parent)
} else {
for (line in line1:line2) {
siblings <- xml2::xml_children(parent)
lines[line] <- paste(
xml2::xml_text(siblings[xml2::xml_attr(siblings, "line1") == line]),
collapse = ""
)
}
}
}
brio::write_lines(lines, path)
```
After all this, we write the `lines`, modified and not, to the original `path`.
It's important to first try this on a script and check the diff.
## Put it all together
Here's the all script, including automatic commit generation.
````{=html}
```{r, eval=FALSE}
parse_script <- function(path) {
cli::cli_alert_info("Refactoring {path}.")
lines <- brio::read_lines(path)
xml <- path |>
parse(keep.source = TRUE) |>
xmlparsedata::xml_parse_data(pretty = TRUE) |>
xml2::read_xml()
deprecated <- xml2::xml_find_all(
xml,
".//SYMBOL_FUNCTION_CALL[text()='expect_that']"
)
purrr::walk(deprecated, treat_deprecated, path = path)
for (deprecated_call in deprecated) {
parent <- xml2::xml_parent(xml2::xml_parent(deprecated_call))
line1 <- as.numeric(xml2::xml_attr(parent, "line1"))
line2 <- as.numeric(xml2::xml_attr(parent, "line2"))
if (line1 == line2) {
lines[line1] <- xml2::xml_text(parent)
} else {
for (line in line1:line2) {
siblings <- xml2::xml_children(parent)
lines[line] <- paste(
xml2::xml_text(siblings[xml2::xml_attr(siblings, "line1") == line]),
collapse = ""
)
}
}
}
brio::write_lines(lines, path)
if (! (path %in% gert::git_status()[["file"]])) {
return(invisible(TRUE))
}
styler::style_file(path)
gert::git_add(path)
gert::git_commit(
sprintf("refactor: remove deprecated expect_that() from %s", fs::path_file(path))
)
}
treat_deprecated <- function(xml, path) {
siblings <- xml2::xml_parent(xml) |> xml2::xml_siblings()
equal <- siblings[grepl("equals\\(", xml2::xml_text(siblings))]
if (length(equal) == 0) {
cli::cli_alert_warning("WARNING AT {path}.")
return()
}
xml2::xml_text(xml) <- "expect_equal"
text <- xml2::xml_contents(equal)[[3]] |> xml2::xml_text()
xml2::xml_remove(xml2::xml_contents(equal))
xml2::xml_text(equal) <- text
}
paths <- fs::dir_ls("tests/testthat", regex = "test-")
purrr::walk(paths, parse_script)
```
````
[Example PR](https://github.com/igraph/rigraph/pull/1337)
## Conclusion
In this post I presented a strategy that served me well when refactoring igraph's test scripts: parsing code to XML, editing it as XML, then writing back the edited lines thanks to the attributes of XML nodes that indicate their original lines in the script.
Other possible approaches include styler's parsing of code into a table and [serialization](https://github.com/r-lib/styler) of that table.
In a more similar approach, which means it might have been wise for me to explore this codebase sooner :sweat_smile:, the codegrip package uses [xmlparsedata](https://github.com/lionel-/codegrip/blob/82d164ed91ad819587796a4053b1df5b0385c677/R/ast.R#L1) and has [helpers for finding which lines a node refers to](https://github.com/lionel-/codegrip/blob/82d164ed91ad819587796a4053b1df5b0385c677/R/ast.R#L51).
Do you sometimes use automatic refactoring ([styler](https://styler.r-lib.org/), [codegrip](https://github.com/lionel-/codegrip), etc.), or automatic diagnostics ([lintr](https://lintr.r-lib.org/), [pkgcheck](https://docs.ropensci.org/pkgcheck/), etc.)?
Have you written any customization or standalone script to help you with that?