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

Document and clarify line ending behaviour #183

Merged
merged 5 commits into from
Jun 27, 2024
Merged

Document and clarify line ending behaviour #183

merged 5 commits into from
Jun 27, 2024

Conversation

hadley
Copy link
Member

@hadley hadley commented Jun 26, 2024

The existing strategy is to keep the newlines that are present. I made only one small change for consistency: parse_all("")$src now returns "" rather than character(). I don't think this should affect knitr (since I think a chunk would need at least a newline in it).

In a future PR, I'll try removing the special handling of the final newline. I think it's safe to eliminate this because we're already pretending that the user pushed enter at the end of the line. But it might affect knitr, so wanted to keep it separate.

The existing strategy is to keep the newlines that are present. I made only one small change for consistency: `parse_all("")$src` now returns `""` rather than `character()`.
@hadley hadley requested a review from cderv June 26, 2024 21:38
@cderv
Copy link
Collaborator

cderv commented Jun 27, 2024

Still some problems on knitr-example with this PR.

Could you resolve the conflict on this PR to merge main into this, so that I can test latest updated version

Not sure what it is, but somehow some empty lines are missing. So something with multiple like \n\n probably replaced by a single one \n

patch of diff after rendering
diff --git a/007-text-output.md b/007-text-output.md
index 38f9128..92e7335 100644
--- a/007-text-output.md
+++ b/007-text-output.md
@@ -27,8 +27,6 @@ for (i in 1:10) {
 
 ``` r
 # two blank lines below
-
-
 dnorm(0)
 ```
 
@@ -77,8 +75,6 @@ dnorm(0)
 
 ``` r
 > # two blank lines below
-> 
-> 
 > dnorm(0)
 ```
 
@@ -174,8 +170,6 @@ for (i in 1:10) {
 
 ``` r
 # two blank lines below
-
-
 dnorm(0)
 ```
 
@@ -200,8 +194,6 @@ dnorm(0)
 
 ``` r
 # two blank lines below
-
-
 dnorm(0)
 ```
 
@@ -328,8 +320,6 @@ R> for (i in 1:10) {
 
 ``` r
 R> # two blank lines below
-R> 
-R> 
 R> dnorm(0)
 ```
 
diff --git a/051-flowchart.tex b/051-flowchart.tex
index 999a4cc..09cbbe5 100644
--- a/051-flowchart.tex
+++ b/051-flowchart.tex
@@ -66,7 +66,6 @@
 \begin{alltt}
 \hlcom{## Flowchart examples}
 \hlkwd{par}\hldef{(}\hlkwc{ask} \hldef{=} \hlnum{TRUE}\hldef{)}
-
 \hlcom{## MODELLING DIAGRAM}
 \hldef{mar} \hlkwb{<-} \hlkwd{par}\hldef{(}\hlkwc{mar} \hldef{=} \hlkwd{c}\hldef{(}\hlnum{1}\hldef{,} \hlnum{1}\hldef{,} \hlnum{1}\hldef{,} \hlnum{1}\hldef{))}
 \hlkwd{openplotmat}\hldef{(}\hlkwc{main} \hldef{=} \hlsng{"from Soetaert and herman, book in prep"}\hldef{,}
@@ -78,29 +77,22 @@
     \hlkwc{arr.side} \hldef{=} \hlnum{3}\hldef{,} \hlkwc{endhead} \hldef{=} \hlnum{TRUE}\hldef{)}
 \hlkwd{segmentarrow}\hldef{(elpos[}\hlnum{7}\hldef{, ], elpos[}\hlnum{4}\hldef{, ],} \hlkwc{arr.pos} \hldef{=} \hlnum{0.15}\hldef{,} \hlkwc{dd} \hldef{=} \hlnum{0.3}\hldef{,}
     \hlkwc{arr.side} \hldef{=} \hlnum{3}\hldef{,} \hlkwc{endhead} \hldef{=} \hlnum{TRUE}\hldef{)}
-
 \hldef{pin} \hlkwb{<-} \hlkwd{par}\hldef{(}\hlsng{"pin"}\hldef{)}  \hlcom{# size of plotting region, inches}
 \hldef{xx} \hlkwb{<-} \hlnum{0.2}
 \hldef{yy} \hlkwb{<-} \hldef{xx} \hlopt{*} \hldef{pin[}\hlnum{1}\hldef{]}\hlopt{/}\hldef{pin[}\hlnum{2}\hldef{]} \hlopt{*} \hlnum{0.15}  \hlcom{# used to make circles round}
-
 \hldef{sx} \hlkwb{<-} \hlkwd{rep}\hldef{(xx,} \hlnum{8}\hldef{)}
 \hldef{sx[}\hlnum{7}\hldef{]} \hlkwb{<-} \hlnum{0.05}
-
 \hldef{sy} \hlkwb{<-} \hlkwd{rep}\hldef{(yy,} \hlnum{8}\hldef{)}
 \hldef{sy[}\hlnum{6}\hldef{]} \hlkwb{<-} \hldef{yy} \hlopt{*} \hlnum{1.5}
 \hldef{sy[}\hlnum{7}\hldef{]} \hlkwb{<-} \hldef{sx[}\hlnum{7}\hldef{]} \hlopt{*} \hldef{pin[}\hlnum{1}\hldef{]}\hlopt{/}\hldef{pin[}\hlnum{2}\hldef{]}
-
 \hlkwa{for} \hldef{(i} \hlkwa{in} \hlkwd{c}\hldef{(}\hlnum{1}\hlopt{:}\hlnum{7}\hldef{))} \hlkwd{straightarrow}\hldef{(}\hlkwc{to} \hldef{= elpos[i} \hlopt{+} \hlnum{1}\hldef{, ],} \hlkwc{from} \hldef{= elpos[i,}
     \hldef{],} \hlkwc{lwd} \hldef{=} \hlnum{2}\hldef{,} \hlkwc{arr.pos} \hldef{=} \hlnum{0.6}\hldef{,} \hlkwc{endhead} \hldef{=} \hlnum{TRUE}\hldef{)}
 \hldef{lab} \hlkwb{<-} \hlkwd{c}\hldef{(}\hlsng{"Problem"}\hldef{,} \hlsng{"Conceptual model"}\hldef{,} \hlsng{"Mathematical model"}\hldef{,}
     \hlsng{"Parameterisation"}\hldef{,} \hlsng{"Mathematical solution"}\hldef{,} \hlsng{""}\hldef{,} \hlsng{"OK?"}\hldef{,} \hlsng{"Prediction, Analysis"}\hldef{)}
-
 \hlkwa{for} \hldef{(i} \hlkwa{in} \hlkwd{c}\hldef{(}\hlnum{1}\hlopt{:}\hlnum{5}\hldef{,} \hlnum{8}\hldef{))} \hlkwd{textround}\hldef{(elpos[i, ], sx[i], sy[i],} \hlkwc{lab} \hldef{= lab[i])}
-
 \hlkwd{textround}\hldef{(elpos[}\hlnum{6}\hldef{, ], xx, yy} \hlopt{*} \hlnum{1.5}\hldef{,} \hlkwc{lab} \hldef{=} \hlkwd{c}\hldef{(}\hlsng{"Calibration,sensitivity"}\hldef{,}
     \hlsng{"Verification,validation"}\hldef{))}
 \hlkwd{textdiamond}\hldef{(elpos[}\hlnum{7}\hldef{, ], sx[}\hlnum{7}\hldef{], sy[}\hlnum{7}\hldef{],} \hlkwc{lab} \hldef{= lab[}\hlnum{7}\hldef{])}
-
 \hlkwd{textplain}\hldef{(}\hlkwd{c}\hldef{(}\hlnum{0.7}\hldef{, elpos[}\hlnum{2}\hldef{,} \hlnum{2}\hldef{]), yy} \hlopt{*} \hlnum{2}\hldef{,} \hlkwc{lab} \hldef{=} \hlkwd{c}\hldef{(}\hlsng{"main components"}\hldef{,}
     \hlsng{"relationships"}\hldef{),} \hlkwc{font} \hldef{=} \hlnum{3}\hldef{,} \hlkwc{adj} \hldef{=} \hlkwd{c}\hldef{(}\hlnum{0}\hldef{,} \hlnum{0.5}\hldef{))}
 \hlkwd{textplain}\hldef{(}\hlkwd{c}\hldef{(}\hlnum{0.7}\hldef{, elpos[}\hlnum{3}\hldef{,} \hlnum{2}\hldef{]), yy,} \hlsng{"general theory"}\hldef{,} \hlkwc{adj} \hldef{=} \hlkwd{c}\hldef{(}\hlnum{0}\hldef{,}
@@ -114,7 +106,6 @@
 \includegraphics[width=\maxwidth]{figure/051-flowchart-demo-flowchart-1} 
 \begin{kframe}\begin{alltt}
 \hlcom{##### DIAGRAM}
-
 \hlkwd{par}\hldef{(}\hlkwc{mar} \hldef{=} \hlkwd{c}\hldef{(}\hlnum{1}\hldef{,} \hlnum{1}\hldef{,} \hlnum{1}\hldef{,} \hlnum{1}\hldef{))}
 \hlkwd{openplotmat}\hldef{()}
 \hldef{elpos} \hlkwb{<-} \hlkwd{coordinates}\hldef{(}\hlkwd{c}\hldef{(}\hlnum{1}\hldef{,} \hlnum{1}\hldef{,} \hlnum{2}\hldef{,} \hlnum{4}\hldef{))}
@@ -141,7 +132,6 @@
 \hlkwd{textellipse}\hldef{(elpos[}\hlnum{8}\hldef{, ],} \hlnum{0.1}\hldef{,} \hlnum{0.1}\hldef{,} \hlkwc{lab} \hldef{=} \hlkwd{c}\hldef{(}\hlsng{"new"}\hldef{,} \hlsng{"article"}\hldef{),}
     \hlkwc{box.col} \hldef{=} \hlsng{"orange"}\hldef{,} \hlkwc{shadow.col} \hldef{=} \hlsng{"red"}\hldef{,} \hlkwc{shadow.size} \hldef{=} \hlnum{0.005}\hldef{,}
     \hlkwc{cex} \hldef{=} \hlnum{1.5}\hldef{)}
-
 \hldef{dd} \hlkwb{<-} \hlkwd{c}\hldef{(}\hlnum{0}\hldef{,} \hlnum{0.025}\hldef{)}
 \hlkwd{text}\hldef{(arrpos[}\hlnum{2}\hldef{,} \hlnum{1}\hldef{]} \hlopt{+} \hlnum{0.05}\hldef{, arrpos[}\hlnum{2}\hldef{,} \hlnum{2}\hldef{],} \hlsng{"yes"}\hldef{)}
 \hlkwd{text}\hldef{(arrpos[}\hlnum{3}\hldef{,} \hlnum{1}\hldef{]} \hlopt{-} \hlnum{0.05}\hldef{, arrpos[}\hlnum{3}\hldef{,} \hlnum{2}\hldef{],} \hlsng{"no"}\hldef{)}
@@ -159,19 +149,16 @@
 \hlkwd{treearrow}\hldef{(}\hlkwc{from} \hldef{= elpos[}\hlnum{1}\hlopt{:}\hlnum{2}\hldef{, ],} \hlkwc{to} \hldef{= elpos[}\hlnum{3}\hlopt{:}\hlnum{5}\hldef{, ],} \hlkwc{arr.side} \hldef{=} \hlnum{2}\hldef{,}
     \hlkwc{path} \hldef{=} \hlsng{"H"}\hldef{)}
 \hlkwa{for} \hldef{(i} \hlkwa{in} \hlnum{1}\hlopt{:}\hlnum{5}\hldef{)} \hlkwd{textrect}\hldef{(elpos[i, ],} \hlnum{0.15}\hldef{,} \hlnum{0.05}\hldef{,} \hlkwc{lab} \hldef{= i,} \hlkwc{cex} \hldef{=} \hlnum{1.5}\hldef{)}
-
 \hlkwd{openplotmat}\hldef{()}
 \hldef{elpos} \hlkwb{<-} \hlkwd{coordinates}\hldef{(}\hlkwd{c}\hldef{(}\hlnum{3}\hldef{,} \hlnum{2}\hldef{),} \hlkwc{hor} \hldef{=} \hlnum{FALSE}\hldef{)}
 \hlkwd{treearrow}\hldef{(}\hlkwc{from} \hldef{= elpos[}\hlnum{1}\hlopt{:}\hlnum{3}\hldef{, ],} \hlkwc{to} \hldef{= elpos[}\hlnum{4}\hlopt{:}\hlnum{5}\hldef{, ],} \hlkwc{arr.side} \hldef{=} \hlnum{2}\hldef{,}
     \hlkwc{arr.pos} \hldef{=} \hlnum{0.2}\hldef{,} \hlkwc{path} \hldef{=} \hlsng{"V"}\hldef{)}
 \hlkwa{for} \hldef{(i} \hlkwa{in} \hlnum{1}\hlopt{:}\hlnum{5}\hldef{)} \hlkwd{textrect}\hldef{(elpos[i, ],} \hlnum{0.15}\hldef{,} \hlnum{0.05}\hldef{,} \hlkwc{lab} \hldef{= i,} \hlkwc{cex} \hldef{=} \hlnum{1.5}\hldef{)}
-
 \hlkwd{openplotmat}\hldef{()}
 \hldef{elpos} \hlkwb{<-} \hlkwd{coordinates}\hldef{(}\hlkwd{c}\hldef{(}\hlnum{1}\hldef{,} \hlnum{4}\hldef{))}
 \hlkwd{treearrow}\hldef{(}\hlkwc{from} \hldef{= elpos[}\hlnum{1}\hldef{, ],} \hlkwc{to} \hldef{= elpos[}\hlnum{2}\hlopt{:}\hlnum{5}\hldef{, ],} \hlkwc{arr.side} \hldef{=} \hlnum{2}\hldef{,}
     \hlkwc{arr.pos} \hldef{=} \hlnum{0.7}\hldef{,} \hlkwc{path} \hldef{=} \hlsng{"H"}\hldef{)}
 \hlkwa{for} \hldef{(i} \hlkwa{in} \hlnum{1}\hlopt{:}\hlnum{5}\hldef{)} \hlkwd{textrect}\hldef{(elpos[i, ],} \hlnum{0.05}\hldef{,} \hlnum{0.05}\hldef{,} \hlkwc{lab} \hldef{= i,} \hlkwc{cex} \hldef{=} \hlnum{1.5}\hldef{)}
-
 \hlkwd{openplotmat}\hldef{()}
 \hldef{elpos} \hlkwb{<-} \hlkwd{coordinates}\hldef{(}\hlkwd{c}\hldef{(}\hlnum{2}\hldef{,} \hlnum{1}\hldef{,} \hlnum{2}\hldef{,} \hlnum{3}\hldef{))}
 \hldef{elpos[}\hlnum{1}\hldef{,} \hlnum{1}\hldef{]} \hlkwb{<-} \hlnum{0.3}
@@ -188,7 +175,6 @@
 \includegraphics[width=\maxwidth]{figure/051-flowchart-demo-flowchart-3} 
 \begin{kframe}\begin{alltt}
 \hlkwd{par}\hldef{(}\hlkwc{mfrow} \hldef{=} \hlkwd{c}\hldef{(}\hlnum{1}\hldef{,} \hlnum{1}\hldef{))}
-
 \hlkwd{par}\hldef{(}\hlkwc{mar} \hldef{=} \hlkwd{c}\hldef{(}\hlnum{0}\hldef{,} \hlnum{0}\hldef{,} \hlnum{0}\hldef{,} \hlnum{0}\hldef{))}
 \hlkwd{openplotmat}\hldef{()}
 \hldef{elpos} \hlkwb{<-} \hlkwd{coordinates}\hldef{(}\hlkwd{c}\hldef{(}\hlnum{1}\hldef{,} \hlnum{1}\hldef{,} \hlnum{2}\hldef{,} \hlnum{1}\hldef{))}
@@ -229,9 +215,7 @@
     \hlkwc{lcol} \hldef{=} \hlnum{6}\hldef{)}
 \hlkwd{splitarrow}\hldef{(}\hlkwc{from} \hldef{= elpos[}\hlnum{1}\hldef{, ],} \hlkwc{to} \hldef{= elpos[}\hlnum{2}\hlopt{:}\hlnum{3}\hldef{, ],} \hlkwc{lty} \hldef{=} \hlnum{1}\hldef{,} \hlkwc{lwd} \hldef{=} \hlnum{1}\hldef{,}
     \hlkwc{dd} \hldef{=} \hlnum{0.7}\hldef{,} \hlkwc{arr.side} \hldef{=} \hlnum{1}\hlopt{:}\hlnum{2}\hldef{,} \hlkwc{lcol} \hldef{=} \hlnum{7}\hldef{)}
-
 \hlkwa{for} \hldef{(i} \hlkwa{in} \hlnum{1}\hlopt{:}\hlnum{4}\hldef{)} \hlkwd{textrect}\hldef{(elpos[i, ],} \hlnum{0.05}\hldef{,} \hlnum{0.05}\hldef{,} \hlkwc{lab} \hldef{= i,} \hlkwc{cex} \hldef{=} \hlnum{1.5}\hldef{)}
-
 \hlkwd{legend}\hldef{(}\hlsng{"topright"}\hldef{,} \hlkwc{lty} \hldef{=} \hlnum{1}\hlopt{:}\hlnum{7}\hldef{,} \hlkwc{legend} \hldef{=} \hlkwd{c}\hldef{(}\hlsng{"segmentarrow"}\hldef{,} \hlsng{"curvedarrow"}\hldef{,}
     \hlsng{"straightarrow"}\hldef{,} \hlsng{"treearrow"}\hldef{,} \hlsng{"bentarrow"}\hldef{,} \hlsng{"selfarrow"}\hldef{,} \hlsng{"splitarrow"}\hldef{),}
     \hlkwc{lwd} \hldef{=} \hlkwd{c}\hldef{(}\hlkwd{rep}\hldef{(}\hlnum{2}\hldef{,} \hlnum{6}\hldef{),} \hlnum{1}\hldef{),} \hlkwc{col} \hldef{=} \hlnum{1}\hlopt{:}\hlnum{7}\hldef{)}
diff --git a/093-knitr-asy.md b/093-knitr-asy.md
index 18136f7..a84d135 100644
--- a/093-knitr-asy.md
+++ b/093-knitr-asy.md
@@ -62,7 +62,6 @@ In this example, I  generate data in R and then use it for plotting in asymptote
 ``` r
 x = seq(0, 5, l = 100)
 y = sin(x)
-
 # save data to csv file
 write.table(data.frame(x, y), file = "asy.csv", col.names = FALSE, row.names = FALSE,
     sep = ",")

@hadley
Copy link
Member Author

hadley commented Jun 27, 2024

Merge conflict fixed, but I can't reproduce the problem in pure evaluate. This returns the same result in both this branch and the main branch:

parse_all(function() {
  # Two blank lines below


  dnorm(0)
})

@cderv
Copy link
Collaborator

cderv commented Jun 27, 2024

but I can't reproduce the problem in pure evaluate.

Yes I noticed this. It happens in specific context. I don't know which one yet, and still need to reproduce. I need to debug this in knitr-example.

@hadley
Copy link
Member Author

hadley commented Jun 27, 2024

In case it helps, I can't reproduce it with a simple .Rmd either:

```{r}
# Two blank lines below


dnorm(0)
```

knitr::knitr() yields this .md:


``` r
# Two blank lines below


dnorm(0)
```

```
## [1] 0.3989423
```

@hadley
Copy link
Member Author

hadley commented Jun 27, 2024

Possibly informative: these lines don't appear in the diff. So it's not every chunk that's losing the empty lines.

@hadley
Copy link
Member Author

hadley commented Jun 27, 2024

I figured out that I could reproduce it with tidy = TRUE and then through a process of careful elimination, I reduced it to this

```{r}
{}


# comment
```

When I knit that I get:


``` r
{}
```

```
## NULL
```

``` r
# comment
```

@hadley
Copy link
Member Author

hadley commented Jun 27, 2024

But I still can't turn that into an evaluate test case, as this code reveals no differences:

code <- "{}\n\n\n# comment"

pr_resume("parse-refactor")
devtools::load_all()
cur <- evaluate(code)

pr_pause()
devtools::load_all()
new <- evaluate(code)

waldo::compare(cur, new)

Maybe it's something to do with how knitr chops up chunks?

@hadley
Copy link
Member Author

hadley commented Jun 27, 2024

Hmmmm, I seem the same behaviour CRAN evaluate so that might not be the root cause 😭

@hadley
Copy link
Member Author

hadley commented Jun 27, 2024

Here's a more obvious reprex:

```{r}
{invisible()}
# should be followed by three blank lines



The top-level {} seem to be an important part of the problem. I see this in CRAN/dev knitr + CRAN evaluate, so I'm not sure why this PR would cause you to see it for the first time. Possibly it's a different bug.

@hadley
Copy link
Member Author

hadley commented Jun 27, 2024

Ok, this seems to be the bug we're seeing here:

```{r demo, tidy = TRUE}
for (i in 1) {}
# two blank lines below


1
```

And that gives me a legit evaluate example:

code <- c("for (i in 1) {\n}", "# two blank lines below", "", "", "1")
parse_all(code)$src

Even simpler reprex:

code <- c("\n", "", "", "1")
parse_all(code)$src
#> [1] "\n" "1" 

@hadley
Copy link
Member Author

hadley commented Jun 27, 2024

And should be fixed now 😄

@cderv
Copy link
Collaborator

cderv commented Jun 27, 2024

This is no more erroring now on knitr-example side. So all good to merge.

tidy = TRUE will trigger formatR::tidy_source() and this is what is adding the new lines with {}

> formatR::tidy_source(text = "{}\n\n# a comment")
{
}

# a comment

@hadley
Copy link
Member Author

hadley commented Jun 27, 2024

@cderv the trick bit is where the formatting is happening and how knitr is sending the code to evaluate — I don't think it's possible to recreate with actual code passed to evaluate.

@hadley hadley merged commit 8fcb061 into main Jun 27, 2024
13 checks passed
@hadley hadley deleted the parse-refactor branch June 27, 2024 21:04
@cderv
Copy link
Collaborator

cderv commented Jun 28, 2024

formatR applies before evaluation. So this would be similar to this without knitr I believe

evaluate::evaluate(formatR::tidy_source(text = "{}\n\n# a comment")$text.tidy)

CRAN evaluate and Dev evaluate now gives the same results for this.

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

Successfully merging this pull request may close these issues.

2 participants