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

2 bugs in make.report #21

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

2 bugs in make.report #21

wants to merge 1 commit into from

Conversation

tokami
Copy link
Member

@tokami tokami commented Jan 29, 2017

There are two major bugs in make.report :
First, specifying a path and file name in the argument reportfile, does not work. I propose following changes, which allow specifying a path only, a path with file name, a file name only, or an empty string for the argument reportfile (as is was stated in the help document). Note that dirname(reportfile) == "." is necessary as the dirname of a file name only, results in a period, which is identified as a valid path.
Second, when the file name contains a space in one of the directory names, the system function does not provide the correct path name to the console. This problem was caused on my Mac, but I think "\ " for escaping space with a backslash is universal across OS.

There are two major bugs in `make.report` :
First, specifying a path and file name in the argument `reportfile`, does not work. I propose following changes, which allow specifying a path only, a path with file name, a file name only, or an empty string for the argument `reportfile` (as is was stated in the help document). Note that `dirname(reportfile) == "."` is necessary as the `dirname` of a file name only, results in a period, which is identified as a valid path. 
Second, when the file name contains a space in one of the directory names, the `system` function does not provide the correct path name to the console. This problem was caused on my Mac, but I think "\ " for escaping space with a backslash is universal across OS.
if(file.access(dirname(reportfile), mode=1) != 0) reportfile <- file.path(getwd(), reportfile)

# if file name specified
if("tex" %in% unlist(strsplit(basename(reportfile),"[.]"))){
Copy link
Member

Choose a reason for hiding this comment

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

The test here could be: grepl(".+[.](tex)$", reportfile)

if(basename(reportfile) == "") reportfile <- 'report.tex'
if(file.access(dirname(reportfile), mode=1) != 0) reportfile <- file.path(getwd(), reportfile)

# if file name specified
Copy link
Member

Choose a reason for hiding this comment

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

There are too many comments and not all of them say what the code does. E.g. here the code checks if "tex" is between dots in the reportfile string, not if a file is specified. Small difference, but maybe crucial.

@@ -101,8 +123,12 @@ make.report <- function(rep, reporttitle='', reportfile='report.tex', summaryout

# -- Compile tex file -- #
#latexcompile <- system(paste('pdflatex -output-directory=../res/', reportfile), intern=TRUE)
latexcompile <- system(paste(paste0('pdflatex -output-directory=',file.path(dirname(reportfile))), file.path(reportfile)), intern=TRUE)
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't we just put the file path and the file name in quotes and remove the double paste?
latexcompile <- system(paste0('pdflatex -output-directory="', file.path(dirname(reportfile)), '" "', file.path(reportfile), '"'))

@@ -40,8 +40,30 @@ latex.figure <- function(figfile, reportfile, caption=''){
#' @return Nothing.
Copy link
Member

Choose a reason for hiding this comment

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

This function returns TRUE or invisible(NULL) depending on !keep.figurefiles, not nothing! But maybe it should return something like the pdf filename with full path, so one could open it using system(paste(getOption("pdfviewer"), outpdf), wait = FALSE). We can also have an argument to show the pdf file after creation.

@tokami tokami added the bug label Nov 28, 2019
@tokami tokami marked this pull request as draft December 3, 2020 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants