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

Bump roxygen2 version #2510

Closed
wants to merge 5 commits into from
Closed

Bump roxygen2 version #2510

wants to merge 5 commits into from

Conversation

IndrajeetPatil
Copy link
Collaborator

Re-documenting with the new version of {roxygen2} (7.3.0) seems to generate a non-parsable NAMESPACE file.

I am wondering if this is a bug upstream.

@MichaelChirico
Copy link
Collaborator

cc @hadley is this an {roxygen2} regression? Here's the markup leading to this garbled output:

lintr/R/lintr-package.R

Lines 20 to 24 in bad1632

#' if (getRversion() >= "4.0.0") {
#' importFrom(tools, R_user_dir)
#' } else {
#' importFrom(backports, R_user_dir)
#' }

@MichaelChirico
Copy link
Collaborator

git bisect points to r-lib/roxygen2@1ea43c9 as the culprit

@AshesITR
Copy link
Collaborator

@MichaelChirico thanks for the pointer. I looked at the diff and found the problem:

update_namespace_imports() sorts the lines, which will break any multi-line @rawNamespace directive.

@MichaelChirico
Copy link
Collaborator

update_namespace_imports

Thanks for the pointer; filed something that works in r-lib/roxygen2#1573.

@IndrajeetPatil
Copy link
Collaborator Author

Thank you, both, for getting to the bottom of this so quickly and for filing a fix!

@codecov-commenter
Copy link

codecov-commenter commented Jan 22, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (538440d) 98.53% compared to head (73f7e37) 98.55%.
Report is 32 commits behind head on main.

❗ Current head 73f7e37 differs from pull request most recent head c37d61e. Consider uploading reports for the commit c37d61e to get more accurate results

Files Patch % Lines
R/shared_constants.R 75.00% 1 Missing ⚠️
R/zzz.R 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2510      +/-   ##
==========================================
+ Coverage   98.53%   98.55%   +0.02%     
==========================================
  Files         125      126       +1     
  Lines        5609     5762     +153     
==========================================
+ Hits         5527     5679     +152     
- Misses         82       83       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@IndrajeetPatil
Copy link
Collaborator Author

Can confirm that devel-roxygen2 fixes the issue.

The new warning in the examples looks legit, though, and something we should deal with.

@MichaelChirico
Copy link
Collaborator

The new warning in the examples looks legit, though, and something we should deal with.

Can you point to the warning? I'm seeing various issues & can't figure out what's the problem, exactly. Maybe we're seeing something different, though.

@IndrajeetPatil
Copy link
Collaborator Author

I see:

> ### Name: get_r_string
> ### Title: Extract text from 'STR_CONST' nodes
> ### Aliases: get_r_string
> 
> ### ** Examples
> 
> ## Don't show: 
> if (requireNamespace("withr", quietly = TRUE)) (if (getRversion() >= "3.4") withAutoprint else force)({ # examplesIf
+ ## End(Don't show)
+ tmp <- withr::local_tempfile(lines = "c('a', 'b')")
+ expr_as_xml <- get_source_expressions(tmp)$expressions[[1L]]$xml_parsed_content
+ writeLines(as.character(expr_as_xml))
+ get_r_string(expr_as_xml, "expr[2]") # "a"
+ get_r_string(expr_as_xml, "expr[3]") # "b"
+ 
+ # more importantly, extract strings under R>=4 raw strings
+ ## Don't show: 
+ }) # examplesIf
> tmp <- withr::local_tempfile(lines = "c('a', 'b')")
> expr_as_xml <- get_source_expressions(tmp)$expressions[[1L]]$xml_parsed_content
Error in file(con, "r") : 
  (converted from warning) cannot open file '/var/folders/xr/v_vddzvs33q5wg7jv9mr__4h0000gn/T//Rtmpk8Dcx3/file640749057d03': No such file or directory
Run `rlang::last_trace()` to see where the error occurred.Loading lintr
> rlang::last_trace()
<error/rlang_error>
Error:
! No such file or directory
---
Backtrace:1. └─devtools::run_examples(run_donttest = TRUE, run_dontrun = TRUE)
  2.   └─base::lapply(...)
  3.     └─pkgload (local) FUN(X[[i]], ...)
  4.       └─base::source(tmp, echo = !quiet, local = env, max.deparse.length = Inf)
  5.         ├─base::withVisible(eval(ei, envir))
  6.         └─base::eval(ei, envir)
  7.           └─base::eval(ei, envir)
  8.             ├─(if (getRversion() >= "3.4") withAutoprint else force)(...) at /Rtmpk8Dcx3/file6407739f7efc.R:8:1
  9.             │ └─base::source(...)
 10.             │   ├─base::withVisible(eval(ei, envir))
 11.             │   └─base::eval(ei, envir)
 12.             │     └─base::eval(ei, envir)
 13.             └─lintr::get_source_expressions(tmp)
 14.               └─lintr:::read_lines(filename) at lintr/R/get_source_expressions.R:69:5
 15.                 ├─base::withCallingHandlers(...) at lintr/R/utils.R:182:3
 16.                 └─base::readLines(file, warn = TRUE, ...)
 17.                   └─base::file(con, "r")

@MichaelChirico
Copy link
Collaborator

Hmm I'm not seeing that. Maybe transient. Closing + reopening to see if 7.3.1 gets picked up by CI yet.

@MichaelChirico
Copy link
Collaborator

#2518 proves (IINM) the issue is not related to {roxygen2}, since running with {pkgload} skips the document=TRUE step {devtools} does.

The new error might then be related to the {pkgload} update. Anyway, merging this.

@MichaelChirico MichaelChirico marked this pull request as ready for review January 25, 2024 03:03
@IndrajeetPatil
Copy link
Collaborator Author

No longer relevant after #2519 merge

@IndrajeetPatil IndrajeetPatil deleted the bump-roxygen-730 branch January 27, 2024 17:14
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.

None yet

4 participants