-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat: protect_curly() #76
Conversation
R/asis-nodes.R
Outdated
xml2::xml_find_all(body, i, ns = ns) | ||
} | ||
|
||
fix_curly <- function(curly) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clearly my regex level is not on par with this file 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I call BS on this comment! I have never known how to work with the regmatches and gregexpr functions and I commend your ability to do this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't even remember how I got it to work tbh 😹
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good! It works as it says on the tin and will catch most of the cases. I'm also thinking that this could be generalised into a function that can take a generic opening pattern and closing pattern to give a specific label so that people can extend it for their own purposes.
There are a couple of cases where the solution at this stage would produce undesired results:
- If attributes are split across lines, it will only find the first line:
The solution is to search one of the following siblings of the nodes for a closing brace as I've done in pegboard.
[a span with attributes]{.span-with-attributes style='color: red;'}
- Images with alt text will be captured
I'm not sure what the solution to this might be, but I suspect we would need to extract this alt text and put it in an attribute.
![caption](img.png){alt='alternative text for the image'}
I'd be happy to try my hand at this, but I'm also happy to merge this as an "in progress" feature before I make an update to CRAN in a couple of months.
R/asis-nodes.R
Outdated
xml2::xml_find_all(body, i, ns = ns) | ||
} | ||
|
||
fix_curly <- function(curly) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I call BS on this comment! I have never known how to work with the regmatches and gregexpr functions and I commend your ability to do this!
Co-authored-by: Zhian N. Kamvar <[email protected]>
Co-authored-by: Zhian N. Kamvar <[email protected]>
Co-authored-by: Zhian N. Kamvar <[email protected]>
glue::glue("./{close_xpath}"), | ||
ns | ||
) | ||
xml2::xml_text(not_closed) <- paste( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should I add a test for the md -> xml + protect_curly() -> md round? If so is there an example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we should absolutely test this to make sure nothing on the xslt side suddenly changes. I think the easiest way to do this would be to use a file in a temporary directory and check the snapshot of the file:
tinkr/tests/testthat/test-class-yarn.R
Lines 53 to 63 in a577e9b
test_that("a yarn object can be written back to markdown", { | |
tmpdir <- withr::local_tempdir() | |
scarf1 <- withr::local_file(file.path(tmpdir, "yarn.md")) | |
scarf2 <- withr::local_file(file.path(tmpdir, "yarn.Rmd")) | |
y1 <- yarn$new(pathmd) | |
y2 <- yarn$new(pathrmd) | |
y1$write(scarf1) | |
y2$write(scarf2) | |
expect_snapshot_file(scarf1) | |
expect_snapshot_file(scarf2) | |
}) |
Also, this is a clever solution 🤩! I had not thought about smashing together text into a text node separated by a newline like this! I always made them separate text nodes, but this solution solves so many problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not sure whether it was a clever or lazy solution to be honest 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YES! Thank you for the quick changes. The only thing left is the tests!
glue::glue("./{close_xpath}"), | ||
ns | ||
) | ||
xml2::xml_text(not_closed) <- paste( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we should absolutely test this to make sure nothing on the xslt side suddenly changes. I think the easiest way to do this would be to use a file in a temporary directory and check the snapshot of the file:
tinkr/tests/testthat/test-class-yarn.R
Lines 53 to 63 in a577e9b
test_that("a yarn object can be written back to markdown", { | |
tmpdir <- withr::local_tempdir() | |
scarf1 <- withr::local_file(file.path(tmpdir, "yarn.md")) | |
scarf2 <- withr::local_file(file.path(tmpdir, "yarn.Rmd")) | |
y1 <- yarn$new(pathmd) | |
y2 <- yarn$new(pathrmd) | |
y1$write(scarf1) | |
y2$write(scarf2) | |
expect_snapshot_file(scarf1) | |
expect_snapshot_file(scarf2) | |
}) |
Also, this is a clever solution 🤩! I had not thought about smashing together text into a text node separated by a newline like this! I always made them separate text nodes, but this solution solves so many problems.
<text curly="true" alt="a picture of a dog">{#dog alt="a picture | ||
of a dog"}</text> | ||
<text/> | ||
<softbreak/> | ||
</paragraph> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this softbreak was originally between the text nodes, but I think it's benign
|
||
![a pretty kitten](https://placekitten.com/200/300){#kitteh alt='a picture of a kitten'} | ||
|
||
![a pretty puppy](https://placedog.net/200/300){#dog alt="a picture |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing space at the end.
of a dog"} | ||
|
||
|
||
\[a span with attributes\]{.span-with-attributes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this escaping expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, yes: r-lib/commonmark#20
I'm going to try to implement this today. It should be a matter of backtracking and finding the square braces that were not escaped in the original document and then tagging them with 'asis' tags.
Co-authored-by: Zhian N. Kamvar <[email protected]>
|
||
\[a span with attributes\]{.span-with-attributes | ||
style='color: red;'} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why so many new lines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it's because of the <softbreak>
: https://github.com/ropensci/tinkr/pull/76/files/3e133e5f6e6b35befef6d8fad20df6d87f8ad549?w=0#r973214227. I guess I was wrong in calling it benign -_-
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to merge this for now given the current limitations and then work on fixing the unescaped square braces.
|
||
\[a span with attributes\]{.span-with-attributes | ||
style='color: red;'} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it's because of the <softbreak>
: https://github.com/ropensci/tinkr/pull/76/files/3e133e5f6e6b35befef6d8fad20df6d87f8ad549?w=0#r973214227. I guess I was wrong in calling it benign -_-
of a dog"} | ||
|
||
|
||
\[a span with attributes\]{.span-with-attributes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, yes: r-lib/commonmark#20
I'm going to try to implement this today. It should be a matter of backtracking and finding the square braces that were not escaped in the original document and then tagging them with 'asis' tags.
So I'll let you finish this PR? Alternatively I can work on it again later this week. Thank you!! |
I've opened #78 to fix the unescaped braces issue, and I think the extra space is a minor issue that we can fix before the CRAN release, so I'm going to merge this. |
Fix #75
I'll be happy to learn while working on this PR 😸