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

Better error messages for tag parsing failures #771

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@BarkleyBG
Copy link

BarkleyBG commented Jul 24, 2018

This attempts to fix #770

Please take a look at the way I am wrangling and passing file_name and section_name through.

This did not break any existing tests, but I think it's perhaps a little more fragile than I'd like. I'd be happy for suggestions - maybe it's a good idea to create a wrangleSectionName() function and add some tests to make sure the class(x)[1] works as intended?

BarkleyBG added some commits Jul 24, 2018

More verbose error handling in as_html()
Fixes #770

I tried to make the error messages extremely verbose by passing in information about the file name and section tag from where the error originated. This may be a little fragile, but it didn't break any existing tests.

- Added unit test for as_html.tag_url
- Made errors more verbose by changing stopifnot() to if-then with a descriptive stop message in multiple cases (e.g., as_html.tag_url, as_html.tag_eqn, etc)
- Also trying to pass in information on the Section and File from which the error was thrown.
@hadley

This comment has been minimized.

Copy link
Member

hadley commented Nov 6, 2018

You shouldn't be passing them in ..., but retrieving them from the global context. (I'm just spinning up on pkgdown again, so will be able to give better advice in a couple of hours when I've refamiliarised myself with the code).

If I can give you some concrete advice are you interested in finishing this PR off?

R/rd-html.R Outdated
@@ -100,7 +109,17 @@ as_html.tag_subsection <- function(x, ...) {

#' @export
as_html.tag_eqn <- function(x, ..., mathjax = TRUE) {
stopifnot(length(x) <= 2)
if ( length(x) > 2 ) {
dots <- list(...)

This comment has been minimized.

@hadley

hadley Nov 6, 2018

Member
  • I think this should use context_get("rdname") to get the name of the input file.

  • I don't think it's worth changing the interface of the function in order to add the section name, we can tackle that in the future.

  • Since you've repeated the same code multiple times, please extract it out into a function

@BarkleyBG

This comment has been minimized.

Copy link
Author

BarkleyBG commented Nov 11, 2018

Thanks for the tips! Here's what I will try to do:

Input file name

I think this should use context_get("rdname") to get the name of the input file.

Instead of passing out$filename as input to functions, I'll grab file names with context_get("rdname") from within functions

Section name

I don't think it's worth changing the interface of the function in order to add the section name, we can tackle that in the future.

I'll omit any use of section_name, and discard that functionality

Function

Since you've repeated the same code multiple times, please extract it out into a function

I'll merge all of the repeated code into a subfunction as best I can!

BarkleyBG added some commits Nov 11, 2018

omit section_name logging
passes all tests in  testthat::test_file("tests/testthat/test-rd-html.R")
create stop_on_bad_html_tag() fun
- Combines similar code into one function
- Passes all 67 tests in testthat::test_file("tests/testthat/test-rd-html.R")
  - Note that the test uses a call to rlang::bind_env(), which helped me to make sure the function worked as desired. This may however have some side effects that I dont know about.

## Binding the rdname to the context environment allows
# stop_on_bad_html_tag() to grab the filename via rlang
rlang::env_bind(.env = context, rdname = "test-rd-html.R")

This comment has been minimized.

@BarkleyBG

BarkleyBG Nov 11, 2018

Author

Re #771, @hadley could you please verify that this call to env_bind() won't have any undesirable side effects? I added it to ensure that the tests throw the correct error message, but the line can be omittted if you wish. Thank you!

This comment has been minimized.

@hadley

hadley Nov 12, 2018

Member

You can use scoped_file_context() which was designed for this exact scenario 😄

@BarkleyBG

This comment has been minimized.

Copy link
Author

BarkleyBG commented Nov 11, 2018

I've made changes that should resolve the comments you've brought up. There are two outstanding issues:

  • merge conflict in R/rd-html.r. Should I rebase on master?
  • I'd like a review of therlang::bind_env() call in the test-rd-html.R file. I think it's helpful, but it can also be removed from the PR.
@hadley
Copy link
Member

hadley left a comment

Looks like you've made good progress! I've made a few more comments.


## Binding the rdname to the context environment allows
# stop_on_bad_html_tag() to grab the filename via rlang
rlang::env_bind(.env = context, rdname = "test-rd-html.R")

This comment has been minimized.

@hadley

hadley Nov 12, 2018

Member

You can use scoped_file_context() which was designed for this exact scenario 😄

)

## Empty URLs throw errors
expect_error( rd2html(x = "\\url{}") )

This comment has been minimized.

@hadley

hadley Nov 12, 2018

Member

You need to include a fragment of the error message to match on to ensure that you're finding the expected error.

@@ -185,7 +207,9 @@ as_html.tag_link <- function(x, ...) {

#' @export
as_html.tag_linkS4class <- function(x, ...) {
stopifnot(length(x) == 1)
if ( length(x) != 1 ) {

This comment has been minimized.

@hadley

hadley Nov 12, 2018

Member

Can you please remove the extra spaces inside the parens? (https://style.tidyverse.org/syntax.html#parentheses)

@hadley hadley changed the title Iss770 tag url failure Better error messages for tag parsing failures Nov 12, 2018

@hadley

This comment has been minimized.

Copy link
Member

hadley commented Nov 12, 2018

Yes, please do merge/rebase to resolve the merge conflict. Also note that I changed the PR title to be more informative (and as general rule, you should never include an issue number in the PR title)

@BarkleyBG

This comment has been minimized.

Copy link
Author

BarkleyBG commented Nov 12, 2018

Great! Thanks for all the help - it's very instructive. I'll try to have this done this week, but feel free to ping me if it becomes urgent.

jayhesselberth added a commit that referenced this pull request Nov 14, 2018

Bad Rd tags throw errors
Cleaned up version of #771. Closes #771.

jayhesselberth added a commit that referenced this pull request Nov 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment