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

ensure lintr runs/installs/tests on R-3.6 #395

Merged
merged 4 commits into from
Aug 16, 2019

Conversation

russHyde
Copy link
Collaborator

@russHyde russHyde commented Aug 15, 2019

The dev version of xmlparsedata has been updated to fix a parsing bug on R-3.6; before fixing that bug several lintr tests were failing (#384). This change points lintr to the dev version of xmlparsedata

@russHyde russHyde requested a review from jimhester as a code owner August 15, 2019 13:26
@russHyde
Copy link
Collaborator Author

russHyde commented Aug 15, 2019

I'm not quite sure what the problem is here. I suspect the function %||% needs rewriting:

`%||%` <- function(x, y) {                                                        
  if (is.null(x) || is.na(x) || length(x) <= 0) {                                 
    y                                                                             
  } else {                                                                        
    x                                                                             
  }                                                                               
}
===> ?
`%||%` <- function(x, y) {                                                        
  if (is.null(x) || any(is.na(x)) || length(x) <= 0) {                                 
    y                                                                             
  } else {                                                                        
    x                                                                             
  }                                                                               
}

Relates to this change in R3.6 NEWS "Experimentally, setting environment variable R_CHECK_LENGTH_1_LOGIC2 will lead to warnings (or errors if the variable is set to a ‘true’ value) when && or || encounter and use arguments of length more than one." [https://cran.r-project.org/doc/manuals/r-devel/NEWS.html]?

@jimhester
Copy link
Member

Maybe this, though we might want to rethink having the is.na() in there at all...

`%||%` <- function(x, y) {                                                        
  if (is.null(x) || length(x) <= 0 || is.na(x[[1L]])) {                                 
    y                                                                             
  } else {                                                                        
    x                                                                             
  }                                                                               
}

@russHyde russHyde changed the title use github-version of xmlparsedata ensure lintr runs/installs/tests on R-3.6 Aug 15, 2019
@russHyde
Copy link
Collaborator Author

Modified the definition of %||%. This should fix #377

@russHyde
Copy link
Collaborator Author

russHyde commented Aug 15, 2019

A similar problem has occurred with the function exclude(). Within the following statement:

file %in% names(exclusions) &&
        exclusions[[file]] == Inf ||
        df$line_number[i] %in% exclusions[[file]]

Presumably exclusions[[file]] can have length 0 or > 1.

Suggest:

file %in% names(exclusions) &&
    length(exclustions[[file]]) == 1L &&
    exclusions[[file]] == Inf ||
    df$line_number[i] %in% exclusions[[file]]

I'm not sure why these errors aren't being detected in my R 3.6.1 environment when I run devtools::test()

@codecov
Copy link

codecov bot commented Aug 15, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@b5dff71). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master    #395   +/-   ##
========================================
  Coverage          ?   82.2%           
========================================
  Files             ?      41           
  Lines             ?    1871           
  Branches          ?       0           
========================================
  Hits              ?    1538           
  Misses            ?     333           
  Partials          ?       0
Impacted Files Coverage Δ
R/exclude.R 93.75% <100%> (ø)
R/utils.R 71.95% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5dff71...b4f007f. Read the comment docs.

@russHyde
Copy link
Collaborator Author

This should fix #384 and #391 and #377

Also, #363 related to failing tests in master, so should be fixed now too

@jimhester
Copy link
Member

Thanks you can merge this whenever.

@russHyde russHyde merged commit 2b1bccd into r-lib:master Aug 16, 2019
@russHyde
Copy link
Collaborator Author

Cheers jim

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.

2 participants