Fix latent bugs and reduce code smells#31
Merged
Merged
Conversation
- openmp_uninstall(): reference the configure_makevars argument instead of an undefined variable that errored after removing files. - tar_package_install(): treat any non-zero exit as failure (was status < 0, which missed real tar failures). - dmg_package_install(): unmount the volume before aborting so a failed install does not leak the mount. - xcode_cli_install(): remove the temp file before aborting (was unreachable after cli_abort). - gfortran(): carry the error message in $stdout so callers stop silently dropping it. - macos_rtools_install()/uninstall(): advance the progress bar via id=/set= instead of passing the handle as inc. - configure_openmp_makevars(): actually return early when ~/.R cannot be created.
Drop 14 unreachable return() statements that followed cli::cli_abort() (which always throws) across gfortran.R, xcode.R, and openmp.R. Also make block_replace() actually print the block it tells the user to copy instead of discarding the paste0() result.
- Add r_version_full() and route the 6 inline paste(R.version...) call sites through it (and is_r_version's full-version branch). - Use system_os() instead of re-inlining tolower(Sys.info()[['sysname']]) in assert_mac() and .onAttach(). - Add arch_switch() for the arm64/aarch64/x86_64 dispatch + shared 'not recognized' error, collapsing four duplicated switch() blocks in installers.R. - Add assert_supported_r_version_for() for the unsupported-R-version guard duplicated across three installer helpers.
- Break the 265-line macos_rtools_install() into rtools_install_announce/xcode_cli/gfortran/recipes/summary helpers; the orchestrator is now a short, readable sequence with the same behavior and output. - Extract the softwareupdate label pipeline in xcode_cli_install() into xcode_cli_available_label(). - Rewrite the toolchain tests to target the new step seams instead of mocking base-R internals, and add test-openmp.R (OpenMP was previously untested).
Move the R 4.0-4.2 architecture dispatch (Intel Mojave DMG, arm64 gfortran 11/12, and the unsupported-arch/old-R aborts) into install_gfortran_legacy(), flattening gfortran_install() from 142 to ~100 lines. Add a focused test covering each legacy dispatch branch.
Run devtools::document() to add man pages for the new internal version helpers and migrate RoxygenNote to Config/roxygen2/version (8.0.0).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A cleanup pass over the existing code, in self-contained commits:
openmp_uninstall()referenced an undefined variable (erroring after removing files),tar_package_install()only caught negative exit codes, a failed DMG install leaked its mount, the install progress bar never advanced, andgfortran()dropped its error message.return()calls aftercli_abort().r_version_full(),system_os(),arch_switch(), and an unsupported-R-version guard, removing duplication across the installer helpers.macos_rtools_install()and thegfortran_install()legacy branch into focused, single-purpose helpers.