Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor cargo-processx functions #397

Merged
merged 22 commits into from
Nov 21, 2024
Merged

Conversation

kbvernon
Copy link
Contributor

This PR tries to bring consistency to how functions call processx::run(). The main goal is to unburden this package by letting processx do the heavy lifting wherever possible. Changes include:

  • setting error_on_status = TRUE everywhere
  • adding an echo parameter and passing it to echo_cmd = echo (print cargo command to console) and echo = echo (print stdout to console). The default is TRUE to ensure that maximum information is printed to the console.
  • setting env = get_cargo_envvars(). I'm not sure this one is strictly necessary. I believe the function was originally written to handle issues with rtools and windows, but functions that don't use this still pass CI on all OS.
  • standardize logic for defining wd. This is now done with a single call to rprojroot::find_package_root_file().
  • using jsonlite::parse_json(simplifyDataFrame = TRUE) whenever cargo command returns json formatted strings in stdout. If we can always assume data.frames rather than lists, I think this will simplify future maintenance.

At the same time, I took the liberty of adding standalone type checks, reformatting to ensure scripts past lintr tests, removing magrittr pipes, and replacing redundant code.

Right now, there are seven files with calls to processx::run(), but I only updated four of them:

  • clean.R
  • cran-compliance.R
  • license_note.R
  • read_cargo_metadata.R
  • source.R
  • use_crate.R
  • utils.R

I'm ignoring cran-compliance.R because I think Josiah's recent work will make that code obsolete. I think the myriad issues in source.R are substantial enough that they call for their own PR. And the call in utils.R is only really there to check for third party cargo tools, so maybe not worth messing with.

R/license_note.R Show resolved Hide resolved
R/license_note.R Show resolved Hide resolved
R/license_note.R Show resolved Hide resolved
R/read_cargo_metadata.R Outdated Show resolved Hide resolved
R/license_note.R Outdated Show resolved Hide resolved
R/license_note.R Outdated Show resolved Hide resolved
R/license_note.R Outdated Show resolved Hide resolved
R/license_note.R Show resolved Hide resolved
R/license_note.R Outdated Show resolved Hide resolved
R/license_note.R Outdated Show resolved Hide resolved
R/clean.R Show resolved Hide resolved
R/clean.R Show resolved Hide resolved
R/license_note.R Outdated Show resolved Hide resolved
R/read_cargo_metadata.R Outdated Show resolved Hide resolved
R/use_crate.R Show resolved Hide resolved
R/read_cargo_metadata.R Outdated Show resolved Hide resolved
@JosiahParry
Copy link
Contributor

codcov is being a bit extreme here. We can address the coverage later on. It is actually penalizing this PR for adding more checks funnily enough.

Good work @kbvernon

@JosiahParry JosiahParry enabled auto-merge (squash) November 21, 2024 22:10
@JosiahParry JosiahParry merged commit 3ee8ae3 into extendr:main Nov 21, 2024
18 of 19 checks passed
Copy link

codecov bot commented Nov 21, 2024

Codecov Report

Attention: Patch coverage is 52.80899% with 84 lines in your changes missing coverage. Please review.

Project coverage is 80.31%. Comparing base (d8689d9) to head (d46ee51).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
R/license_note.R 0.00% 69 Missing ⚠️
R/run_cargo.R 72.22% 10 Missing ⚠️
R/clean.R 88.46% 3 Missing ⚠️
R/read_cargo_metadata.R 94.44% 1 Missing ⚠️
R/use_crate.R 95.45% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

Files with missing lines Coverage Δ
R/find_extendr.R 100.00% <ø> (ø)
R/utils.R 100.00% <100.00%> (ø)
R/read_cargo_metadata.R 95.23% <94.44%> (-4.77%) ⬇️
R/use_crate.R 97.61% <95.45%> (-2.39%) ⬇️
R/clean.R 82.50% <88.46%> (+12.32%) ⬆️
R/run_cargo.R 72.22% <72.22%> (ø)
R/license_note.R 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

---- 🚨 Try these New Features:

@kbvernon kbvernon deleted the processx branch November 21, 2024 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants