Dont delete template man dir

martinholub commented 5 years ago

dont delete your template man dir
default value for doc_template_names

hpages commented 5 years ago

@martinholub Thanks for the pull request. Can you provide some context and motivation for it? What issue it addresses, was the issue discussed somewhere, how it can be reproduced etc... Thanks!

martinholub commented 5 years ago

Hi @hpages ,

the small change was done because the function would always try to access doc_template_names variable, although it may not have been created in the first place. Addiitonally, the template man pages in the install location of AnnotationForge would be deleted upon calling makeAnnDbPkg. This indeed could not have been desired behavior.

You can reproduce it with:

  dbdir <- "somedir"
  orgdb_name <- "somename"
  tax_id <- "1234" # NCBI taxonomy ID, pick a correct one
  orgdb <- OrganismDbi:::.taxIdToOrgDb(tax_id)

  db_path_sqlite <- file.path(dbdir, paste0(orgdb_name, ".sqlite"))
  file.copy(orgdb$conn@dbname, db_path_sqlite)

  mdata <- AnnotationDbi::metadata(orgdb)
  seed <- new("AnnDbPkgSeed",
              Package= paste0(orgdb_name, ".db"),
              Version="0.0.1",
              Author="me",
              Maintainer="you",
              PkgTemplate="NOSCHEMA.DB",
              AnnObjPrefix=orgdb_name,
              organism = mdata$value[mdata$name == "ORGANISM"],
              species = mdata$value[mdata$name == "SPECIES"],
              biocViews = "annotation",
              manufacturerUrl = "none",
              manufacturer = "none",
              chipName = "none")

AnnotationForge::makeAnnDbPkg(seed, db_path_sqlite, dest_dir = dbdir, no.man = TRUE)

where no.man can be T or F.

I am not aware of this being discussed somewhere else.

dvantwisk commented 5 years ago

Hi @martinholub,

I've been looking at your proposed change and I thank you for pointing out this issue. I do ask, however, that a few things about this push request be changed:

Your change as it stands would keep an empty man directory if no.man=TRUE. It would be better to simply remove the entire man directory because having a man directory with no files may cause an installation error: https://cran.r-project.org/doc/manuals/r-release/R-exts.html#Package-subdirectories. The doc_template_names <- list() should go to line 360 before the if-else statement, and the unlink(man_dir, recursive=TRUE) should remain in order to remove the man directory.
Please include a version bump with your pull request (pull the most recent commits and bump to version 1.25.1).

dvantwisk commented 5 years ago

@martinholub Did you still want to pursue this pull request?

martinholub commented 5 years ago

Sorry, Things have been busy on my side lately. Yeah, I would like to finish it, will try to do it soon. Thanks for patience.

martinholub commented 5 years ago

Hi @dvantwisk ,

I made the changes you asked for. Hope this closes the PR.

dvantwisk commented 4 years ago

Sorry this took so long. The changes seem good, I will merge the pull request now.

Bioconductor / AnnotationForge

Dont delete template man dir #2