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

Consistently strip trailing \n when parsing #179

Closed
wants to merge 8 commits into from
Closed

Conversation

hadley
Copy link
Member

@hadley hadley commented Jun 24, 2024

I can't tell what the original logic was supposed to be, and it seemed inconsistent. Given that the newline was sometimes present and sometimes absent, I assume downstream code must already deal with the differences so this change won't break anything, but we'll need to double-check that. I have confirmed that all testthat tests still pass and it doesn't affect any snapshot output.

Includes some code quality improvements.

hadley added 4 commits June 24, 2024 17:42
I can't tell what the original logic was supposed to be, and it seemed inconsistent. Given that the newline was sometimes present and sometimes absent, I assume downstream code must already deal with the differences so this change won't break anything, but we'll need to double-check that. I have confirmed that all testthat tests still pass and it doesn't affect any snapshot output.
@hadley hadley requested a review from cderv June 24, 2024 16:57
@hadley
Copy link
Member Author

hadley commented Jun 25, 2024

I've gone a bit further with this refactoring, but I have been careful to not further affect the algorithm, just improve the variable names so it's a bit easier to understand what's happening. I've also added quite a few comments to help me understand what is going on.

Copy link
Collaborator

@cderv cderv left a comment

Choose a reason for hiding this comment

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

I don't know exactly the logic - we would need to dig into commit history and related issues to understand. I know newlines in chunk output for knitr is something sensitive

Though, this PR is currently breaking some knitr examples tests where newlines are expected

Diff patch of examples' changes
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 = ",")

However, I can't reproduce it outside of this context, so I need to check what is happening. I can reproduce it locally with knitr example, but not running evaluate directly

@cderv
Copy link
Collaborator

cderv commented Jun 25, 2024

In fact, I have this change in behavior which is expected by this PR I guess

With CRAN evaluate

> str(evaluate::evaluate(input = "# two blank lines below\n\n\ndnorm(0)"))
List of 5
 $ :List of 1
  ..$ src: chr "# two blank lines below\n"
  ..- attr(*, "class")= chr "source"
 $ :List of 1
  ..$ src: chr "\n"
  ..- attr(*, "class")= chr "source"
 $ :List of 1
  ..$ src: chr "\n"
  ..- attr(*, "class")= chr "source"
 $ :List of 1
  ..$ src: chr "dnorm(0)"
  ..- attr(*, "class")= chr "source"
 $ : chr "[1] 0.3989423\n"

With this PR version

> str(evaluate(input = "# two blank lines below\n\n\ndnorm(0)"))
List of 5
 $ :List of 1
  ..$ src: chr "# two blank lines below"
  ..- attr(*, "class")= chr "source"
 $ :List of 1
  ..$ src: chr ""
  ..- attr(*, "class")= chr "source"
 $ :List of 1
  ..$ src: chr ""
  ..- attr(*, "class")= chr "source"
 $ :List of 1
  ..$ src: chr "dnorm(0)"
  ..- attr(*, "class")= chr "source"
 $ : chr "[1] 0.3989423\n"
 - attr(*, "class")= chr [1:2] "evaluate_evaluation" "list"

But it seems it will impact knitr which does not seem to add the new lines back everywhere 🤔

Still need to find where

@hadley
Copy link
Member Author

hadley commented Jun 25, 2024

Hmmm, if it's going to break knitr I probably won't merge it. Maybe the rule is that evaluate adds a \n whenever the source would otherwise be blank?

@hadley
Copy link
Member Author

hadley commented Jun 27, 2024

Replaced by #183

@hadley hadley closed this Jun 27, 2024
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