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

Upkeep #22

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Upkeep #22

wants to merge 16 commits into from

Conversation

dunkenwg
Copy link
Contributor

No description provided.

@dunkenwg
Copy link
Contributor Author

dunkenwg commented Oct 31, 2024

resolves poissonconsulting/infrastructure-issues#204

@dunkenwg dunkenwg requested a review from joethorley October 31, 2024 22:26
@dunkenwg
Copy link
Contributor Author

dunkenwg commented Oct 31, 2024

One test in test-knit.R is failing with the following error: '\U' used without hex digits in character string (<text>:3:13).

I'm guessing this has to do with file path differences between operating systems. I think both mac and ubuntu accept \, but windows uses \\ (the first \ is an escape character for the second \). Should we skip this test on windows or remove the windows R-CMD-check?

@joethorley joethorley requested a review from aylapear November 2, 2024 15:33
@aylapear
Copy link
Member

aylapear commented Nov 7, 2024

One test in test-knit.R is failing with the following error: '\U' used without hex digits in character string (<text>:3:13).

I'm guessing this has to do with file path differences between operating systems. I think both mac and ubuntu accept \, but windows uses \\ (the first \ is an escape character for the second \). Should we skip this test on windows or remove the windows R-CMD-check?

It would be good to dig into the error further before turning the test off and seeing if there is a change we can make to the code so it works on both windows and other systems. I think you should dig into the code and determine what exactly is causing the issue in windows and if anything can be done to fix it as it.

@dunkenwg
Copy link
Contributor Author

dunkenwg commented Nov 7, 2024

I think the error is occurring because of this line in knit.R: utils::browseURL(paste0('file://', path)). I tried switching it to utils::browseURL(paste0('file:///', path)) to see if that would fix the problem in windows. It didn't... Do either of you have any suggestions for making the url file path work for windows?

@aylapear
Copy link
Member

aylapear commented Nov 9, 2024

I think the error is occurring because of this line in knit.R: utils::browseURL(paste0('file://', path)). I tried switching it to utils::browseURL(paste0('file:///', path)) to see if that would fix the problem in windows. It didn't... Do either of you have any suggestions for making the url file path work for windows?

Have you tried building the file path with the file.path() function and seen if this helps instead of using paste0()?

@dunkenwg
Copy link
Contributor Author

The Windows R-CMD-check is actually failing on this line in sbr_knit_results():

  path <- rmarkdown::render(
    input = input,
    output_format = "html_document",
    quiet = quiet
  )

I tried adding an argument for the output directory that would differ between systems:

  path <- rmarkdown::render(
    input = input,
    output_format = "html_document",
    output_dir = file.path(dirname(file)),
    quiet = quiet
  )

but we're still getting the file path error.

I could try making if/else statements to run different code on Windows, but I don't really know what else to change after scouring the internet on this error. We could also skip this test on Windows, but users likely wouldn't be able to use sbr_knit_results() on Windows. Thoughts?

@aylapear
Copy link
Member

aylapear commented Dec 3, 2024

image

This is happening because of the path of the main variable in the "results.Rmd" document we are creating for the test. I tested it on a windows machine and if I switch the slashes to be / or to be \\ then it works fine. I think how we are setting up the test is causing this issue and how the files are used in real cases is a bit different so this issue only pertains to the test structure.

I think we can skip on windows but worth filing an issue on this as there may be a different way to set up the test to be more robust on all OS in the futre. I believe this function will work on windows machines.

@aylapear
Copy link
Member

aylapear commented Dec 3, 2024

I was incorrect above, this issue is not just about how the test is set up but the purpose of this function which is to created a quick way to view the tables, plots, figures, etc in the report format but it is not actually generating the report we write up.

To fix this we need to make sure it encodes it properly for windows where the "quick" report structure is created. In the report-text.R file try changing the normalizePath(main) to be normalizePath(main, win slash = "/") and see if this will now pass on windows machines and other OS.

@dunkenwg
Copy link
Contributor Author

dunkenwg commented Dec 5, 2024

@aylapear, this fix worked! If I had to make an (educated) guess as to why test-knit.R is failing on windows when it calls sbr_knit_results(), I'd say it all starts with writeLines(report_text(sub = sub, main = main), con = input) on line 28 of sbr_knit_results(). This line first calls report_text() which writes a really long string. writeLines() then writes this string to results.Rmd. Without the winslash argument in line 11 of report_text() this file fails to render on windows when rmarkdown::render() is called on line 30 of sbr_knit_results().

If I've got the fix correct, you can go ahead and merge the pull request as long as @joethorley is happy with the changes.

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