Declutter: Review all else blocks

[chainsawriot]

Some of them are not needed, e.g.

rio/R/export.R

Lines 81 to 99 in 067a0ae

	if (missing(file) && missing(format)) {
	stop("Must specify 'file' and/or 'format'")
	} else if (!missing(file) && !missing(format)) {
	fmt <- tolower(format)
	cfile <- file
	f <- find_compress(file)
	file <- f$file
	compress <- f$compress
	} else if (!missing(file) && missing(format)) {
	cfile <- file
	f <- find_compress(file)
	file <- f$file
	compress <- f$compress
	fmt <- get_ext(file)
	} else if (!missing(format)) {
	fmt <- get_type(format)
	file <- paste0(as.character(substitute(x)), ".", fmt)
	compress <- NA_character_
	}

    if (missing(file) && missing(format)) {
        stop("Must specify 'file' and/or 'format'")
    }
    if (!missing(file) && !missing(format)) {
        fmt <- tolower(format)
        cfile <- file
        f <- find_compress(file)
        file <- f$file
        compress <- f$compress
    }
    if (!missing(file) && missing(format)) {
        cfile <- file
        f <- find_compress(file)
        file <- f$file
        compress <- f$compress
        fmt <- get_ext(file)
    }
    if (missing(file) && !missing(format)) {
        fmt <- get_type(format)
        file <- paste0(as.character(substitute(x)), ".", fmt)
        compress <- NA_character_
    }

[chainsawriot]

Some dummy else blocks were removed in 44eb387

[schochastics]

@chainsawriot would you consider this decluttered:

    .check_file(file, single_only = TRUE)
    if (missing(file) && missing(format)) {
        stop("Must specify 'file' and/or 'format'")
    }
    if(!missing(file)){
        cfile <- file
        f <- find_compress(file)
        file <- f$file
        compress <- f$compress
        format <- ifelse(!missing(format), tolower(format), get_info(file)$input)
    } else{
        format <- .standardize_format(format)
        file <- paste0(as.character(substitute(x)), ".", format)
        compress <- NA_character_
    }

[chainsawriot]

@schochastics Sure is! Anything reducing LOCs without loss of readibility is a great improvement!

One minor gp bureaucracy: use isFALSE in ifelse because ifelse is vectorized (which we don't want). See also data.table::fifelse.

[schochastics]

@chainsawriot ok thanks. I will keep looking for other if/else declutter parts today and send a PR later

[schochastics]

in export_list,

if (is.null(file)) {
        stop("'file' must be a character vector")
}

is already handled by .check_file(file, single_only = FALSE)

[schochastics]

This line never did what it was supposed to do due to wrong parenthesis

rio/R/export_list.R

Line 64 in 294ba02

if (any(nchar(names(x))) == 0) {

[chainsawriot]

rio/R/export_methods.R

Lines 139 to 152 in 294ba02

	.export.rio_rdata <- function(file, x, ...) {
	if (is.data.frame(x)) {
	return(save(x, file = file, ...))
	} else if (is.list(x)) {
	e <- as.environment(x)
	save(list = names(x), file = file, envir = e, ...)
	} else if (is.environment(x)) {
	save(list = ls(x), file = file, envir = x, ...)
	} else if (is.character(x)) {
	save(list = x, file = file, ...)
	} else {
	stop("'x' must be a data.frame, list, or environment")
	}
	}

[schochastics]

@chainsawriot a lot going on there:

1. I dont think the return is needed?
2. the else if (is.character(x)) block is also not really needed because it leads to unexpected behaviour?

.export.rio_rdata <- function(file, x, ...) {
    if (is.data.frame(x)) {
        return(save(x, file = file, ...))
    } else if (is.list(x)) {
        e <- as.environment(x)
        save(list = names(x), file = file, envir = e, ...)
    } else if (is.environment(x)) {
        save(list = ls(x), file = file, envir = x, ...)
    } else if (is.character(x)) {
        save(list = x, file = file, ...)
    } else {
        stop("'x' must be a data.frame, list, or environment")
    }
}
x <- data.frame( a=14 )
.export.rio_rdata("test.RData","x")
load("test.RData")
x
#> [1] "x"

^{Created on 2023-09-11 with reprex v2.0.2}

[schochastics]

hmm but export("iris","iris.RData") does what it is supposed to do
hmmmm:

library(rio)
export("iris","iris.RData")
x <- data.frame(a = 14)
export("x","x.RData")
head(import("iris.RData"))
#>   Sepal.Length Sepal.Width Petal.Length Petal.Width Species
#> 1          5.1         3.5          1.4         0.2  setosa
#> 2          4.9         3.0          1.4         0.2  setosa
#> 3          4.7         3.2          1.3         0.2  setosa
#> 4          4.6         3.1          1.5         0.2  setosa
#> 5          5.0         3.6          1.4         0.2  setosa
#> 6          5.4         3.9          1.7         0.4  setosa
import("x.RData")
#> [1] "x"

^{Created on 2023-09-11 with reprex v2.0.2}

[chainsawriot]

@schochastics Yea, the .Rdata support is wacky. #295

[schochastics]

@chainsawriot I see. Before I mess something up:
how would you rewrite .export.rio_rdata in that case?
/Edit: like this?

.export.rio_rdata <- function(file, x, ...) {
    if (is.data.frame(x)) {
        save(x, file = file, ...)
    }
    if (is.list(x)) {
        e <- as.environment(x)
        save(list = names(x), file = file, envir = e, ...)
    }
    if (is.environment(x)) {
        save(list = ls(x), file = file, envir = x, ...)
    }
    if (is.character(x)) {
        save(list = x, file = file, ...)
    } else {
        stop("'x' must be a data.frame, list, or environment")
    }
}

[chainsawriot]

.export.rio_rdata <- function(file, x, ...) {
    if (isFALSE(is.data.frame(x)) && isFALSE(is.list(x)) && isFALSE(is.environment(x)) && isFALSE(is.character(x))) {
        stop("'x' must be a data.frame, list, or environment")
    }
    if (is.data.frame(x)) {
        return(save(x, file = file, ...))
    }
    if (is.list(x)) {
        e <- as.environment(x)
        return(save(list = names(x), file = file, envir = e, ...))
    }
    if (is.environment(x)) {
        return(save(list = ls(x), file = file, envir = x, ...))
    }
    return(save(list = x, file = file, ...)) ## characters, but is this doing what it does?
}

But I am card-carrying never nester. I abuse return sometimes.

[schochastics]

ok then I was already close :)

[schochastics]

@chainsawriot are you fine with me rewriting

rio/R/compression.R

Lines 51 to 94 in 81bbf48

	parse_zip <- function(file, which, ...) {
	d <- tempfile()
	dir.create(d)
	file_list <- utils::unzip(file, list = TRUE)
	if (missing(which)) {
	which <- 1
	if (nrow(file_list) > 1) {
	warning(sprintf("Zip archive contains multiple files. Attempting first file."))
	}
	}
	if (is.numeric(which)) {
	utils::unzip(file, files = file_list$Name[which], exdir = d)
	file.path(d, file_list$Name[which])
	} else {
	if (substring(which, 1, 1) != "^") {
	which2 <- paste0("^", which)
	}
	utils::unzip(file, files = file_list$Name[grep(which2, file_list$Name)[1]], exdir = d)
	file.path(d, which)
	}
	}
	parse_tar <- function(file, which, ...) {
	d <- tempfile()
	dir.create(d)
	on.exit(unlink(d))
	file_list <- utils::untar(file, list = TRUE)
	if (missing(which)) {
	which <- 1
	if (length(file_list) > 1) {
	warning(sprintf("Tar archive contains multiple files. Attempting first file."))
	}
	}
	if (is.numeric(which)) {
	utils::untar(file, files = file_list[which], exdir = d)
	file.path(d, file_list[which])
	} else {
	if (substring(which, 1, 1) != "^") {
	which2 <- paste0("^", which)
	}
	utils::untar(file, files = file_list[grep(which2, file_list)[1]], exdir = d)
	file.path(d, which)
	}
	}

to

parse_archive <- function(file, which, file_type, ...) {
    d <- tempfile()
    dir.create(d)

    if (file_type == "zip") {
        file_list <- utils::unzip(file, list = TRUE)$Name
        extract_func <- utils::unzip
    } else if (file_type == "tar") {
        file_list <- utils::untar(file, list = TRUE)
        extract_func <- utils::untar
    } else {
        stop("Unsupported file_type. Use 'zip' or 'tar'.")
    }

    if (missing(which)) {
        if (length(file_list) > 1) {
            warning(sprintf("%s archive contains multiple files. Attempting first file.", file_type))
        }
        which <- 1
    }

    if (is.numeric(which)) {
        extract_func(file, files = file_list[which], exdir = d)
        return(file.path(d, file_list[which]))
    } else {
        if (substring(which, 1, 1) != "^") {
            which2 <- paste0("^", which)
        }
        extract_func(file, files = file_list[grep(which2, file_list)[1]], exdir = d)
        return(file.path(d, which))
    }
}

and making the relevant adjustments in import?
Many redundancies in those 2 functions

[chainsawriot]

@schochastics I like that. I would stop upfront when file_type is not zip or tar. So that d will not create in that case.

Maybe one less else?

    if (is.numeric(which)) {
        extract_func(file, files = file_list[which], exdir = d)
        return(file.path(d, file_list[which]))
    }
    if (substring(which, 1, 1) != "^") {
        which2 <- paste0("^", which)
    }
    extract_func(file, files = file_list[grep(which2, file_list)[1]], exdir = d)
    return(file.path(d, which))
    }