pkgtools/R2pkg: refactoring, tests

2 files changed, 133 insertions, 119 deletions

diff --git a/pkgtools/R2pkg/files/R2pkg.R b/pkgtools/R2pkg/files/R2pkg.R
index 95580e98a2f..a9cddec94e8 100644
--- a/pkgtools/R2pkg/files/R2pkg.R
+++ b/pkgtools/R2pkg/files/R2pkg.R
@@ -1,4 +1,4 @@
-# $NetBSD: R2pkg.R,v 1.14 2019/10/18 22:10:46 rillig Exp $
+# $NetBSD: R2pkg.R,v 1.15 2019/10/19 11:04:46 rillig Exp $
 #
 # Copyright (c) 2014,2015,2016,2017,2018,2019
 #	Brook Milligan.  All rights reserved.
@@ -747,7 +747,6 @@ make.new_license <- function(df,license)
   df
 }
 
-license.marked.todo <- function(todo) { todo != '' }
 license.in.pkgsrc <- function(license) { license %in% sapply(licenses,'[',1) }
 
 make.license <- function(df)
@@ -757,7 +756,10 @@ make.license <- function(df)
   old_todo <- element(df,'LICENSE','old_todo')
   new_license <- element(df,'LICENSE','new_value')
 
-  if (license.in.pkgsrc(old_license) && license.in.pkgsrc(new_license))
+  old_known <- license.in.pkgsrc(old_license)
+  new_known <- license.in.pkgsrc(new_license)
+
+  if (old_known && new_known)
     {
       if (case.insensitive.equals(old_license,new_license))
         {
@@ -770,17 +772,17 @@ make.license <- function(df)
           todo <- old_todo
         }
     }
-  else if (license.in.pkgsrc(old_license) && !license.in.pkgsrc(new_license))
+  else if (old_known && !new_known)
     {
       license <- paste0(old_license,'\t# [R2pkg] updated to: ',new_license)
       todo <- '# TODO: '
     }
-  else if (!license.in.pkgsrc(old_license) && license.in.pkgsrc(new_license))
+  else if (!old_known && new_known)
     {
       license <- paste0(new_license,'\t# [R2pkg] previously: ',old_license)
       todo <- ''
     }
-  else if (!license.in.pkgsrc(old_license) && !license.in.pkgsrc(new_license))
+  else
     {
       license <- paste0(new_license,'\t# [R2pkg] previously: ',old_license)
       todo <- '# TODO: '
@@ -803,11 +805,11 @@ find.order <- function(df,key,field)
   value
 }
 
-update.Makefile.with.metadata <- function(df,metadata)
+mklines.update_with_metadata <- function(df, metadata)
 {
   df$new_value <- NA
 
-  df <- make.new_license(df,metadata$License)
+  df <- make.new_license(df, metadata$License)
 
   df$new_value[df$key == 'CATEGORIES'] <- categories()
   df$new_value[df$key == 'MAINTAINER'] <- arg.maintainer_email
@@ -816,9 +818,8 @@ update.Makefile.with.metadata <- function(df,metadata)
   df
 }
 
-update.Makefile.with.new.values <- function(df)
+mklines.update_value <- function(df)
 {
-  # message('===> update.Makefile.with.new.values():')
   df$value <- NA
   df$todo <- ''
   df <- make.license(df)
@@ -826,88 +827,41 @@ update.Makefile.with.new.values <- function(df)
   df$value[df$key == 'MAINTAINER'] <- make.maintainer(df)
   df$value[df$key == 'COMMENT'] <- make.comment(df)
   df$value[df$key == 'R_PKGVER'] <- make.r_pkgver(df)
-
-  # str(df)
-  # print(df)
-  df
-}
-
-update.Makefile.with.new.line <- function(df)
-{
-  # message('===> update.Makefile.with.new.line():')
-  df$new_line <- NA
-
-  construct_key_value <- df$key_value & !is.na(df$value)
-  df$new_line[construct_key_value] <-
-    paste0(df$todo[construct_key_value],
-           df$key[construct_key_value],
-           df$operator[construct_key_value],
-           df$delimiter[construct_key_value],
-           df$value[construct_key_value])
-
-  copy_line <- !df$key_value | !construct_key_value
-  df$new_line[copy_line] <- df$line[copy_line]
-
-  # str(df)
-  # print(df)
-  df
-}
-
-annotate.distname.in.Makefile <- function(df)
-{
-  match <- grepl('^[[:blank:]]*DISTNAME',df$new_line)
-  line <- df$new_line[match]
-  value <- sub('^[[:blank:]]*DISTNAME[[:blank:]]*=[[:blank:]]*','',line)
-  pkgname <- sub('_.+$','',value)
-  pkgver <- sub('^.+_','',value)
-  PKGNAME <- paste0('R_PKGNAME=',pkgname)
-  PKGVER <- paste0('R_PKGVER=',pkgver)
-  comment <- paste0('\t# [R2pkg] replace this line with ',PKGNAME,' and ',PKGVER,' as first stanza')
-  df$new_line[match] <- paste0(line,comment)
   df
 }
 
-annotate.Makefile <- function(df)
+mklines.update_new_line <- function(df)
 {
-  df <- annotate.distname.in.Makefile(df)
-  df
-}
+  df$new_line <- df$line
 
-remove.master.sites.from.Makefile <- function(df)
-{
-  match <- grepl('^[[:blank:]]*MASTER_SITES',df$new_line)
-  df <- df[!match,]
-  df
-}
+  va <- df$key_value & !is.na(df$value)
+  df$new_line[va] <- paste0(
+    df$todo[va], df$key[va], df$operator[va], df$delimiter[va], df$value[va])
 
-remove.homepage.from.Makefile <- function(df)
-{
-  match <- grepl('^[[:blank:]]*HOMEPAGE',df$new_line)
-  df <- df[!match,]
   df
 }
 
-remove.buildlink.abi.depends.from.Makefile <- function(df)
+mklines.annotate_distname <- function(mklines)
 {
-  match <- grepl('^[[:blank:]]*BUILDLINK_ABI_DEPENDS',df$new_line)
-  df <- df[!match,]
-  df
+  match <- grepl('^[[:blank:]]*DISTNAME', mklines$new_line)
+  line <- mklines$new_line[match]
+  value <- sub('^[[:blank:]]*DISTNAME[[:blank:]]*=[[:blank:]]*', '', line)
+  comment <- sprintf(
+    '\t# [R2pkg] replace this line with %s and %s as first stanza',
+    paste0('R_PKGNAME=', sub('_.+$', '', value)),
+    paste0('R_PKGVER=', sub('^.+_', '', value)))
+  mklines$new_line[match] <- paste0(line, comment)
+  mklines
 }
 
-remove.buildlink.api.depends.from.Makefile <- function(df)
+mklines.remove_lines_before_update <- function(mklines)
 {
-  match <- grepl('^[[:blank:]]*BUILDLINK_API_DEPENDS',df$new_line)
-  df <- df[!match,]
-  df
-}
-
-remove.lines.from.Makefile <- function(df)
-{
-  df <- remove.master.sites.from.Makefile(df)
-  df <- remove.homepage.from.Makefile(df)
-  df <- remove.buildlink.abi.depends.from.Makefile(df)
-  df <- remove.buildlink.api.depends.from.Makefile(df)
-  df
+  remove <- (
+    grepl('^[[:blank:]]*MASTER_SITES', mklines$new_line) |
+    grepl('^[[:blank:]]*HOMEPAGE', mklines$new_line) |
+    grepl('^[[:blank:]]*BUILDLINK_ABI_DEPENDS', mklines$new_line) |
+    grepl('^[[:blank:]]*BUILDLINK_API_DEPENDS', mklines$new_line))
+  mklines[!remove,]
 }
 
 reassign.order <- function(df)
@@ -1047,11 +1001,11 @@ update.Makefile <- function(metadata)
 
   # message('===> df:')
   df <- read.Makefile.as.dataframe()
-  df <- update.Makefile.with.metadata(df,metadata)
-  df <- update.Makefile.with.new.values(df)
-  df <- update.Makefile.with.new.line(df)
-  df <- annotate.Makefile(df)
-  df <- remove.lines.from.Makefile(df)
+  df <- mklines.update_with_metadata(df,metadata)
+  df <- mklines.update_value(df)
+  df <- mklines.update_new_line(df)
+  df <- mklines.annotate_distname(df)
+  df <- mklines.remove_lines_before_update(df)
   df <- reassign.order(df)
 
   df.conflicts <- make.df.conflicts(df,metadata)
diff --git a/pkgtools/R2pkg/files/R2pkg_test.R b/pkgtools/R2pkg/files/R2pkg_test.R
index 779cf33e924..58bbc5ade50 100644
--- a/pkgtools/R2pkg/files/R2pkg_test.R
+++ b/pkgtools/R2pkg/files/R2pkg_test.R
@@ -1,4 +1,4 @@
-# $NetBSD: R2pkg_test.R,v 1.9 2019/10/18 22:10:46 rillig Exp $
+# $NetBSD: R2pkg_test.R,v 1.10 2019/10/19 11:04:46 rillig Exp $
 #
 # Copyright (c) 2019
 #	Roland Illig.  All rights reserved.
@@ -36,13 +36,19 @@ library(withr)
 arg.recursive <- FALSE
 arg.update <- FALSE
 
-package_dir <- file.path(Sys.getenv('PKGSRCDIR'), 'pkgtools', 'R2pkg')
+pkgsrcdir <- Sys.getenv('PKGSRCDIR')
+package_dir <- file.path(pkgsrcdir, 'pkgtools', 'R2pkg')
 
 expect_printed <- function(obj, ...) {
     out <- ''
     with_output_sink(textConnection('out', 'w', local = TRUE), print(obj))
-    expect_equal(length(out), length(c(...)))
-    expect_equal(!!out, !!c(...))
+    exp <- c(...)
+    if (length(out) != length(exp) || out != exp) {
+        write(out, 'R2pkg_test.out.txt')
+        write(exp, 'R2pkg_test.exp.txt')
+    }
+    expect_equal(length(out), length(exp))
+    expect_equal(!!out, !!exp)
 }
 
 linesConnection <- function(...)
@@ -559,8 +565,9 @@ test_that('find.order', {
     expect_equal(include_order, NA_integer_)
 })
 
-test_that('update.Makefile.with.metadata', {
+test_that('mklines.update_with_metadata with CATEGORIES', {
     local_dir(package_dir)  # to get a realistic category
+    arg.maintainer_email <<- 'with-categories@example.org'
     df <- read.Makefile.as.dataframe(linesConnection(
         'CATEGORIES=\told categories',
         'MAINTAINER=\told_maintainer@example.org',
@@ -568,63 +575,116 @@ test_that('update.Makefile.with.metadata', {
         'R_PKGVER=\t1.0'))
     metadata = list(Title = 'Package comment', Version = '19.3', License = 'license')
 
-    updated <- update.Makefile.with.metadata(df, metadata)
+    updated <- mklines.update_with_metadata(df, metadata)
 
     expect_printed(data.frame(key = updated$key, new_value = updated$new_value),
-        '         key       new_value',
-        '1 CATEGORIES        pkgtools',
-        '2 MAINTAINER                ',  # FIXME: Should not always be reset.
-        '3    COMMENT Package comment',
-        '4   R_PKGVER            19.3')
+        '         key                   new_value',
+        '1 CATEGORIES                    pkgtools',
+        '2 MAINTAINER with-categories@example.org',  # FIXME: Should not always be reset.
+        '3    COMMENT             Package comment',
+        '4   R_PKGVER                        19.3')
 })
 
 # If the variable has been removed from the Makefile, it is not updated.
-test_that('update.Makefile.with.metadata without CATEGORIES', {
+test_that('mklines.update_with_metadata without CATEGORIES', {
+    arg.maintainer_email <<- 'without-categories@example.org'
     df <- read.Makefile.as.dataframe(linesConnection(
         'MAINTAINER=',
         'COMMENT=',
         'R_PKGVER='))
     metadata = list(Title = 'Package comment', Version = '19.3', License = 'license')
 
-    updated <- update.Makefile.with.metadata(df, metadata)
+    updated <- mklines.update_with_metadata(df, metadata)
 
     expect_printed(updated,
         '         line order category key_value        key depends buildlink3.mk',
         '1 MAINTAINER=     1       NA      TRUE MAINTAINER   FALSE         FALSE',
         '2    COMMENT=     2       NA      TRUE    COMMENT   FALSE         FALSE',
         '3   R_PKGVER=     3       NA      TRUE   R_PKGVER   FALSE         FALSE',
-        '  operator delimiter old_value old_todo       new_value',
-        '1        =                                             ',
-        '2        =                              Package comment',
-        '3        =                                         19.3')
+        '  operator delimiter old_value old_todo                      new_value',
+        '1        =                              without-categories@example.org',
+        '2        =                                             Package comment',
+        '3        =                                                        19.3')
 })
 
-# test_that('update.Makefile.with.new.values', {
-# })
+test_that('mklines.update_value', {
+    local_dir(package_dir)
 
-# test_that('update.Makefile.with.new.line', {
-# })
+    mklines <- read.Makefile.as.dataframe(linesConnection(
+        'R_PKGVER=\t1.0',
+        'CATEGORIES=\told categories',
+        'MAINTAINER=\told_maintainer@example.org',
+        'COMMENT=\tOld comment',
+        'LICENSE=\told-license'
+    ))
+    mklines$new_value <- mklines$old_value
 
-# test_that('annotate.distname.in.Makefile', {
-# })
+    updated <- mklines.update_value(mklines)
 
-# test_that('annotate.Makefile', {
-# })
+    expect_equal(updated$value, c(
+        '1.0',
+        'pkgtools old categories',
+        'old_maintainer@example.org',
+        'Old comment',
+        'old-license\t# [R2pkg] previously: old-license'))  # FIXME: no comment necessary
+    expect_equal(updated$todo, c(
+        '',
+        '',
+        '',
+        '',
+        '# TODO: '))
+})
 
-# test_that('remove.master.sites.from.Makefile', {
-# })
+test_that('mklines.update_new_line', {
+    mklines <- read.Makefile.as.dataframe(linesConnection(
+        'VALUE=\tvalue',
+        'VALUE_WITH_COMMENT=\tvalue # comment',
+        'VALUE_NA=\tvalue',
+        '#COMMENTED=\tcommented value'
+    ))
+    mklines <- mklines.update_value(mklines)
+    mklines$value[mklines$key == 'VALUE'] <- 'new value'
+    mklines$value[mklines$key == 'VALUE_WITH_COMMENT'] <- 'new value # new comment'
+    mklines$value[mklines$key == '#COMMENTED'] <- 'new commented'
 
-# test_that('remove.homepage.from.Makefile', {
-# })
+    updated <- mklines.update_new_line(mklines)
 
-# test_that('remove.buildlink.abi.depends.from.Makefile', {
-# })
+    expect_equal(updated$new_line, c(
+        'VALUE=\tnew value',
+        'VALUE_WITH_COMMENT=\tnew value # new comment',
+        'VALUE_NA=\tvalue',
+        '#COMMENTED=\tnew commented'))
+})
 
-# test_that('remove.buildlink.api.depends.from.Makefile', {
-# })
+test_that('mklines.annotate_distname', {
+    mklines <- read.Makefile.as.dataframe(linesConnection(
+        'DISTNAME=\tpkg_1.0'))
+    mklines$new_line <- mklines$line
 
-# test_that('remove.lines.from.Makefile', {
-# })
+    annotated <- mklines.annotate_distname(mklines)
+
+    expect_equal(
+        annotated$new_line,
+        'DISTNAME=\tpkg_1.0\t# [R2pkg] replace this line with R_PKGNAME=pkg and R_PKGVER=1.0 as first stanza')
+})
+
+test_that('mklines.remove_lines_before_update', {
+    mklines <- read.Makefile.as.dataframe(linesConnection(
+        'MASTER_SITES=',
+        'HOMEPAGE=',
+        'BUILDLINK_API_DEPENDS.dependency+=',
+        'BUILDLINK_ABI_DEPENDS.dependency+=',
+        'COMMENT='))
+    mklines$new_line <- mklines$line
+
+    cleaned <- mklines.remove_lines_before_update(mklines)
+
+    expect_printed(cleaned,
+    '      line order category key_value     key depends buildlink3.mk operator',
+    '5 COMMENT=     5       NA      TRUE COMMENT   FALSE         FALSE        =',
+    '  delimiter old_value old_todo new_line',
+    '5                              COMMENT=')
+})
 
 # test_that('reassign.order', {
 # })