Adressing [PR review](microsoft#6729 (review)).

- `model` and `tree` parameters made mandatory
- fixed linting
- combined tree type tests

Author: fboudry

R-package/R/lgb.plot.tree.R

@@ -102,12 +102,12 @@
 #' @importFrom data.table := fcoalesce fifelse setnames
 #' @importFrom DiagrammeR add_global_graph_attrs create_edge_df create_graph create_node_df render_graph
 #' @export
-lgb.plot.tree <- function(model = NULL,
-                          tree = NULL,
-                          rules = NULL,
-                          render = TRUE,
-                          plot_width = NULL,
-                          plot_height = NULL
+lgb.plot.tree <- function(model
+                          , tree
+                          , rules = NULL
+                          , render = TRUE
+                          , plot_width = NULL
+                          , plot_height = NULL
                           ) {
     # check model is lgb.Booster
     if (!.is_Booster(x = model)) {
@@ -119,12 +119,8 @@ lgb.plot.tree <- function(model = NULL,
             call. = FALSE
         )
     }
-    # tree must be integer or numeric
-    if (!inherits(tree, c('integer','numeric'))) {
-      stop(sprintf("lgb.plot.tree: 'tree' must only contain integers."))
-    }
     # all elements of tree must be integers
-    if (!all(tree %% 1L == 0L)) {
+    if (!inherits(tree, c("integer", "numeric")) || !all(tree %% 1L == 0L)) {
       stop(sprintf("lgb.plot.tree: 'tree' must only contain integers."))
     }
     # extract data.table model structure
@@ -140,9 +136,10 @@ lgb.plot.tree <- function(model = NULL,
     # filter modelDT to just the rows for the selected trees
     modelDT <- modelDT[tree_index %in% tree]
     # change some column names to shorter and more diagram friendly versions
-    data.table::setnames(modelDT
-    , old = c("tree_index", "split_feature", "threshold", "split_gain")
-    , new = c("Tree", "Feature", "Split", "Gain")
+    data.table::setnames(
+      modelDT
+      , old = c("tree_index", "split_feature", "threshold", "split_gain")
+      , new = c("Tree", "Feature", "Split", "Gain")
     )
     # the output from "lgb.model.dt.tree" follows these rules
     # "leaf_value" and "leaf_count" are only populated for leaves (NA for internal splits)

R-package/tests/testthat/test_lgb.plot.tree.R

@@ -27,12 +27,14 @@ for (model_name in names(models)){
   test_that("lgb.plot.tree fails when a non existing tree is selected", {
     expect_error({
       lgb.plot.tree(model, -1L)
-    }, regexp = paste0("lgb.plot.tree: All values of 'tree' should be between 0 and the total number of trees in the model minus one"))
+    }, regexp = paste0(
+      "lgb.plot.tree: All values of 'tree' should be between 0 and the total number of trees in the model minus one"))
   })
   test_that("lgb.plot.tree fails when a non existing tree is selected", {
     expect_error({
       lgb.plot.tree(model, 999L)
-    }, regexp = paste0("lgb.plot.tree: All values of 'tree' should be between 0 and the total number of trees in the model minus one"))
+    }, regexp = paste0(
+      "lgb.plot.tree: All values of 'tree' should be between 0 and the total number of trees in the model minus one"))
   })
   test_that("lgb.plot.tree fails when a non numeric tree is selected", {
     expect_error({