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

Use new warn_roxy_tag() and warn_roxy_block() #1317

Merged
merged 19 commits into from
Apr 8, 2022
Merged

Use new warn_roxy_tag() and warn_roxy_block() #1317

merged 19 commits into from
Apr 8, 2022

Conversation

hadley
Copy link
Member

@hadley hadley commented Apr 5, 2022

These new functions are powered by cli, given considerably better displays.

A few things to note:

  • markdown() now checks for incomplete braces; I think this is ok. Certainly it seems inconsistent to only check it for sectioned md, which is what we were doing before.
  • I overhauled the generated tags so that that include the file and line from the block. I think this is a nice (if minor) improvement; I did it mainly in order to simplify the warning code in try_find_topic_in_package().
  • Converting the tests to use snapshots revealed that a lot of the warning messages were poorly worded and/or ungrammatical.
  • Use warn(parent = err) makes it so much easier to forward on the underlying problem.

To do:

  • Add news bullet
  • Add RStudio hyperlinks

@hadley hadley requested a review from gaborcsardi April 5, 2022 16:24
R/markdown-state.R Outdated Show resolved Hide resolved
R/tag.R Show resolved Hide resolved
R/rd-raw.R Outdated Show resolved Hide resolved
R/markdown-link.R Outdated Show resolved Hide resolved
R/markdown.R Outdated Show resolved Hide resolved
R/rd-r6.R Outdated Show resolved Hide resolved
R/rd-r6.R Outdated Show resolved Hide resolved
R/rd-r6.R Outdated Show resolved Hide resolved
R/rd-r6.R Outdated Show resolved Hide resolved
tests/testthat/test-rd-r6.R Outdated Show resolved Hide resolved
@gaborcsardi
Copy link
Member

Looks great! Not only are the warnings better, the code that generates them is simpler as well. Left some comments, mostly cosmetics.

R/markdown.R Outdated
roxy_tag_warning(tag, "Unknown xml node: ", xml_name(xml))
warn_roxy_tag(tag, c(
"markdown translation failed",
x = "Internal error: unknown xml node {xml_name(xml)}"
Copy link
Member

Choose a reason for hiding this comment

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

We could be more helpful here as well, maybe suggest opening an issue?

@hadley hadley merged commit f94f43e into main Apr 8, 2022
@hadley hadley deleted the cli_warn branch April 8, 2022 14:36
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