DrylandEcology / rSFSW2

rSFSW2: A R package to create soil water balance simulation experiment
GNU General Public License v3.0
8 stars 7 forks source link

Enhancement#305 gridded global datav5 #323

Open nip5 opened 6 years ago

nip5 commented 6 years ago

Merge gridded 250m soil data extraction functionality.

codecov[bot] commented 6 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@cec8014). Click here to learn what that means. The diff coverage is 4.65%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #323   +/-   ##
=========================================
  Coverage          ?   47.67%           
=========================================
  Files             ?       40           
  Lines             ?    16751           
  Branches          ?        0           
=========================================
  Hits              ?     7986           
  Misses            ?     8765           
  Partials          ?        0
Impacted Files Coverage Δ
R/Simulation_Run.R 75.58% <ø> (ø)
R/Miscellaneous_Functions.R 60.44% <ø> (ø)
R/ExtractData_Soils.R 0% <0%> (ø)
R/Simulation_Project.R 71.71% <100%> (ø)
R/Testproject_Functions.R 49.4% <50%> (ø)

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 cec8014...5f2c10d. Read the comment docs.

CaitlinA commented 6 years ago

@dschlaep I have looked over these changes a few times, and presently I have requested a couple of simple changes. Overall, I think it is ready for your review.

dschlaep commented 6 years ago

Yes, line length should be <= 80 characters.

See unit tests in test_rSFSW2_StyleSpelllingPractices.R and lintr settings in .lintr -- this is (mostly) based on Hadley Wickham's R package book (http://r-pkgs.had.co.nz/r.html#style http://r-pkgs.had.co.nz/r.html#style): "Strive to limit your code to 80 characters per line. This fits comfortably on a printed page with a reasonably sized font. If you find yourself running out of room, this is a good indication that you should encapsulate some of the work in a separate function."

On Aug 20, 2018, at 14:26, nip5 notifications@github.com wrote:

@nip5 commented on this pull request.

In demo/SFSW2_project_descriptions.R https://github.com/DrylandEcology/rSFSW2/pull/323#discussion_r211362368:

   "GriddedDailyWeatherFromMaurer2002_NorthAmerica", 0,
  • - Thornton et al. 1997: 1-km res. for 1980-2016; data expected at

  • file.path(project_paths[["dir_ex_weather"]], "DayMet_NorthAmerica",

  • - Thornton et al. 1997: 1-km res. for 1980-2016; data expected at file.path(

  • project_paths[["dir_ex_weather"]], "DayMet_NorthAmerica",

    Oh right, we try and maintain a line length of <= 80 correct?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/DrylandEcology/rSFSW2/pull/323#discussion_r211362368, or mute the thread https://github.com/notifications/unsubscribe-auth/AEAp23q4f8_oGc44ndYrnoNZjLi-VT7lks5uSv9igaJpZM4VxAfO.

CaitlinA commented 6 years ago

@dschlaep Can you walk me through why the codecov is failing? It doesn't make sense to me.

dschlaep commented 6 years ago
CaitlinA commented 6 years ago

Thanks for the rundown.

Besides the codecov failing, I think this branch is ready for your review.

On Thu, Aug 23, 2018 at 1:46 PM, daniel notifications@github.com wrote:

  • the codecov checks are not required, i.e., even though they are failing, the PR can be merged into master
  • the check codecov/patch is failing because 4.47% of the changes are unit tested -- it checks that the changes are at least as thoroughly checked as the current master is (48.01%)
  • the check codecov/project is failing because after merging into master the code coverage will drop by 0.35%

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/DrylandEcology/rSFSW2/pull/323#issuecomment-415565567, or mute the thread https://github.com/notifications/unsubscribe-auth/AM0e_7KoAgYPmgMUXhKDm8IsKQ7QXyDcks5uTxSogaJpZM4VxAfO .

-- Caitlin Andrews Ecologist Southwest Biological Science Center U.S. Geological Survey Mobile: 802-922-3494 Office: 928-556-7036

dschlaep commented 6 years ago

I will review once PR passes our tests, e.g.,

✖ |  0 2   1 | Code style, spell checks, and good package practices [392.4 s]
────────────────────────────────────────────────────────────────────────────────
test_rSFSW2_StyleSpelllingPractices.R:29: failure: Package code style
Not lint free
R/ExtractData_Soils.R:267:33: style: Trailing whitespace is superfluous.
#' dir_ex_soil to an environment 
                                ^
R/ExtractData_Soils.R:309:13: style: Opening curly braces should never go on their own line and should always be followed by a new line.
    on.exit({print(paste0("rSFSW2's ", temp_call, ": ended after ",
            ^
R/ExtractData_Soils.R:313:1: style: Trailing whitespace is superfluous.

^~
R/ExtractData_Soils.R:315:1: style: Trailing whitespace is superfluous.

^~
R/ExtractData_Soils.R:320:5: style: Place a space before left parenthesis, except in a function call.
  if(!file.exists(dir.ex.gridded)) stop(paste0("Folder '", dir.ex.gridded,
    ^
R/ExtractData_Soils.R:322:1: style: Trailing whitespace is superfluous.

^~
R/ExtractData_Soils.R:328:1: style: Trailing whitespace is superfluous.

^~
R/ExtractData_Soils.R:336:1: style: Trailing whitespace is superfluous.

^~
R/ExtractData_Soils.R:337:67: style: Trailing whitespace is superfluous.
  # set this once now to avoid extracting data that already exists  
                                                                  ^~
R/ExtractData_Soils.R:339:1: style: Trailing whitespace is superfluous.

^~
R/ExtractData_Soils.R:340:5: style: Place a space before left parenthesis, except in a function call.
  if(n_extract > 0){
    ^
R/ExtractData_Soils.R:342:18: warning: Avoid 1:length(...) expressions, use seq_len.
    for (tif in (1:length(file_in_gridded))){
                 ^
R/ExtractData_Soils.R:346:9: style: Place a space before left parenthesis, except in a function call.
      if(grepl('.tif$', tif_file)){
        ^
R/ExtractData_Soils.R:346:16: style: Only use double-quotes.
      if(grepl('.tif$', tif_file)){
               ^~~~~~~
R/ExtractData_Soils.R:347:11: style: Place a space before left parenthesis, except in a function call.
        if(grepl('^bd', tif_file)){
          ^
R/ExtractData_Soils.R:347:18: style: Only use double-quotes.
        if(grepl('^bd', tif_file)){
                 ^~~~~
R/ExtractData_Soils.R:350:16: style: Place a space before left parenthesis, except in a function call.
        else if(grepl('^gravel', tif_file)){
               ^
R/ExtractData_Soils.R:350:23: style: Only use double-quotes.
        else if(grepl('^gravel', tif_file)){
                      ^~~~~~~~~
R/ExtractData_Soils.R:352:39: style: Trailing whitespace is superfluous.
          file_type <- "GravelContent" 
                                      ^
R/ExtractData_Soils.R:357:1: style: Trailing whitespace is superfluous.

^~
R/ExtractData_Soils.R:359:1: style: Trailing whitespace is superfluous.

^~~~~~~~
R/ExtractData_Soils.R:364:13: style: Place a space before left parenthesis, except in a function call.
          if(grepl("[[:digit:]]", ltemp[[1]][i])){
            ^
R/ExtractData_Soils.R:372:1: style: Trailing whitespace is superfluous.

^~~~
R/ExtractData_Soils.R:374:1: style: Trailing whitespace is superfluous.

^~~~~~~~
R/ExtractData_Soils.R:383:1: style: Trailing whitespace is superfluous.

^~~~~~
R/ExtractData_Soils.R:385:13: style: Place a space before left parenthesis, except in a function call.
          if(!file.exists(tif_file)) stop(paste0("File '", tif_file,
            ^
R/ExtractData_Soils.R:387:48: style: Trailing whitespace is superfluous.
          layer_N <- length(ldepth_gridded) - 1 
                                               ^
R/ExtractData_Soils.R:389:1: style: Trailing whitespace is superfluous.

^~~~~~
R/ExtractData_Soils.R:395:73: style: Trailing whitespace is superfluous.
            sites_conus <- sp::spTransform(sites_conus, CRS = soil_data) 
                                                                        ^
R/ExtractData_Soils.R:397:1: style: Trailing whitespace is superfluous.

^~~~~~
R/ExtractData_Soils.R:401:1: style: Trailing whitespace is superfluous.

^~~~~~
R/ExtractData_Soils.R:412:1: style: Trailing whitespace is superfluous.

^~~~~~
R/ExtractData_Soils.R:430:80: style: Trailing whitespace is superfluous.
                                                                 x = list(gd))) 
                                                                               ^
R/ExtractData_Soils.R:431:60: style: Use <-, not =, for assignment.
          MMC[["data"]][todos, grep("depth", MMC[["cn"]])] = soil_frame_depth
                                                           ^
R/ExtractData_Soils.R:432:13: style: Place a space before left parenthesis, except in a function call.
          if(file_type != "depth"){
            ^
R/ExtractData_Soils.R:433:43: style: Trailing whitespace is superfluous.
            # get soil data as a dataframe 
                                          ^
R/ExtractData_Soils.R:435:75: style: Trailing whitespace is superfluous.
                                                             x = list(g))) 
                                                                          ^
R/ExtractData_Soils.R:437:1: style: Trailing whitespace is superfluous.

^~~~~~
R/ExtractData_Soils.R:440:15: style: Place a space before left parenthesis, except in a function call.
            if(file_type == "matricd"){
              ^
R/ExtractData_Soils.R:443:80: style: Trailing whitespace is superfluous.
                                        soil_layer] <- soil_frame / percent_div 
                                                                               ^
R/ExtractData_Soils.R:445:20: style: Place a space before left parenthesis, except in a function call.
            else if(file_type == "GravelContent"){
                   ^
R/ExtractData_Soils.R:450:55: style: Commas should always have a space after.
                                  MMC[["cn"]])[ils]][,soil_layer] <- soil_frame 
                                                      ^
R/ExtractData_Soils.R:450:80: style: Trailing whitespace is superfluous.
                                  MMC[["cn"]])[ils]][,soil_layer] <- soil_frame 
                                                                               ^
R/ExtractData_Soils.R:454:72: style: Trailing whitespace is superfluous.
              MMC[["data"]][todos, grep(file_type, MMC[["cn"]])[ils]][, 
                                                                       ^
R/ExtractData_Soils.R:457:1: style: Trailing whitespace is superfluous.

^~~~~~
R/ExtractData_Soils.R:466:1: style: Trailing whitespace is superfluous.

^~~~
R/ExtractData_Soils.R:468:1: style: Trailing whitespace is superfluous.

^~~~
R/ExtractData_Soils.R:469:67: style: Trailing whitespace is superfluous.
    i_good <- stats::complete.cases(MMC[["data"]][todos, "depth"]) 
                                                                  ^
R/ExtractData_Soils.R:474:62: style: Trailing whitespace is superfluous.
      i_Done <- rep(FALSE, times = sim_size[["runsN_sites"]]) 
                                                             ^
R/ExtractData_Soils.R:1048:5: style: Place a space before left parenthesis, except in a function call.
  if(exinfo$ExtractSoilDataFromIsricSoilGrid_Global_250m){
    ^
tests/testthat/test_ExtractData_Soils.R:8:1: style: Lines should not be more than 80 characters.
# instead the goal here is to provide tests that are general enough that changing
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test_ExtractData_Soils.R:9:1: style: Lines should not be more than 80 characters.
# the files from which to extract data from won't cause the tests to fail. Ie. the
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test_ExtractData_Soils.R:10:1: style: Lines should not be more than 80 characters.
# tests are designed to test the underlining structures created from these functions.
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test_ExtractData_Soils.R:26:49: style: Commas should always have a space after.
SWRunInformation <- data.frame(row.names <- c(1,2,3,4,5,6))
                                                ^
tests/testthat/test_ExtractData_Soils.R:26:51: style: Commas should always have a space after.
SWRunInformation <- data.frame(row.names <- c(1,2,3,4,5,6))
                                                  ^
tests/testthat/test_ExtractData_Soils.R:26:53: style: Commas should always have a space after.
SWRunInformation <- data.frame(row.names <- c(1,2,3,4,5,6))
                                                    ^
tests/testthat/test_ExtractData_Soils.R:26:55: style: Commas should always have a space after.
SWRunInformation <- data.frame(row.names <- c(1,2,3,4,5,6))
                                                      ^
tests/testthat/test_ExtractData_Soils.R:26:57: style: Commas should always have a space after.
SWRunInformation <- data.frame(row.names <- c(1,2,3,4,5,6))
                                                        ^
tests/testthat/test_ExtractData_Soils.R:27:1: style: Lines should not be more than 80 characters.
SWRunInformation[,1] <- c(-106.2995, -106.2748, -106.2813, -106.2875, -106.2875, -106.2875)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test_ExtractData_Soils.R:27:19: style: Commas should always have a space after.
SWRunInformation[,1] <- c(-106.2995, -106.2748, -106.2813, -106.2875, -106.2875, -106.2875)
                  ^
tests/testthat/test_ExtractData_Soils.R:28:1: style: Lines should not be more than 80 characters.
SWRunInformation[,2] <- c(35.7655, 35.76451, 35.77990, 35.79530, 35.79530, 35.79530)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test_ExtractData_Soils.R:28:19: style: Commas should always have a space after.
SWRunInformation[,2] <- c(35.7655, 35.76451, 35.77990, 35.79530, 35.79530, 35.79530)
                  ^
tests/testthat/test_ExtractData_Soils.R:29:19: style: Commas should always have a space after.
SWRunInformation[,3] <- rep("GriddedFROM100m", 6)
                  ^
tests/testthat/test_ExtractData_Soils.R:30:19: style: Commas should always have a space after.
SWRunInformation[,4] <- rep.int(1, 6)
                  ^
tests/testthat/test_ExtractData_Soils.R:31:1: style: Lines should not be more than 80 characters.
colnames(SWRunInformation) <- c("X_WGS84", "Y_WGS84", "SoilTexture_source", "Include_YN_SoilSources")
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test_ExtractData_Soils.R:34:30: style: Commas should always have a space after.
sim_size$runIDs_sites <- c(1,2,3,5,6)
                             ^
tests/testthat/test_ExtractData_Soils.R:34:32: style: Commas should always have a space after.
sim_size$runIDs_sites <- c(1,2,3,5,6)
                               ^
tests/testthat/test_ExtractData_Soils.R:34:34: style: Commas should always have a space after.
sim_size$runIDs_sites <- c(1,2,3,5,6)
                                 ^
tests/testthat/test_ExtractData_Soils.R:34:36: style: Commas should always have a space after.
sim_size$runIDs_sites <- c(1,2,3,5,6)
                                   ^
tests/testthat/test_ExtractData_Soils.R:37:31: style: Commas should never have a space before.
sim_size$runIDs_total <- c(1  ,2,  3,  4,  5,  6,  7,  8,  9, 10 ,11, 12)
                            ~~^
tests/testthat/test_ExtractData_Soils.R:37:32: style: Commas should always have a space after.
sim_size$runIDs_total <- c(1  ,2,  3,  4,  5,  6,  7,  8,  9, 10 ,11, 12)
                               ^
tests/testthat/test_ExtractData_Soils.R:37:66: style: Commas should never have a space before.
sim_size$runIDs_total <- c(1  ,2,  3,  4,  5,  6,  7,  8,  9, 10 ,11, 12)
                                                                ~^
tests/testthat/test_ExtractData_Soils.R:37:67: style: Commas should always have a space after.
sim_size$runIDs_total <- c(1  ,2,  3,  4,  5,  6,  7,  8,  9, 10 ,11, 12)
                                                                  ^
tests/testthat/test_ExtractData_Soils.R:43:37: style: Commas should always have a space after.
sim_size$runIDs_sites_by_dbW <- c(1,2,3,4,5)
                                    ^
tests/testthat/test_ExtractData_Soils.R:43:39: style: Commas should always have a space after.
sim_size$runIDs_sites_by_dbW <- c(1,2,3,4,5)
                                      ^
tests/testthat/test_ExtractData_Soils.R:43:41: style: Commas should always have a space after.
sim_size$runIDs_sites_by_dbW <- c(1,2,3,4,5)
                                        ^
tests/testthat/test_ExtractData_Soils.R:43:43: style: Commas should always have a space after.
sim_size$runIDs_sites_by_dbW <- c(1,2,3,4,5)
                                          ^
tests/testthat/test_ExtractData_Soils.R:49:1: style: Lines should not be more than 80 characters.
sim_space$run_sites <- sp::SpatialPoints(coords <- SWRunInformation[sim_size[["runIDs_sites"]],
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test_ExtractData_Soils.R:51:1: style: Lines should not be more than 80 characters.
                                                 proj4string <- sim_space[["crs_sites"]])
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test_ExtractData_Soils.R:66:1: style: Lines should not be more than 80 characters.
input_soils_use_colnames <- list("Matricd_L", "GravelContent_L", "EvapCoeff_L", "Grass_TranspCoeff_L", "Shrub_TranspCoeff_L", "Tree_TranspCoeff_L", "Forb_TranspCoeff_L",
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test_ExtractData_Soils.R:67:1: style: Lines should not be more than 80 characters.
                                "TranspRegion_L", "Sand_L", "Clay_L", "TOC_GperKG_L", "Imperm_L", "SoilTemp_L")
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test_ExtractData_Soils.R:73:62: style: Commas should always have a space after.
    newList[j] <- paste0(input_soils_use_colnames[[counter]],i)
                                                             ^
tests/testthat/test_ExtractData_Soils.R:74:7: style: Place a space before left parenthesis, except in a function call.
    if(counter >= length(input_soils_use_colnames)) {
      ^
tests/testthat/test_ExtractData_Soils.R:88:51: style: Commas should always have a space after.
sw_input_soillayers <- data.frame(row.names = c(1,2,3,4,5,6))
                                                  ^
tests/testthat/test_ExtractData_Soils.R:88:53: style: Commas should always have a space after.
sw_input_soillayers <- data.frame(row.names = c(1,2,3,4,5,6))
                                                    ^
tests/testthat/test_ExtractData_Soils.R:88:55: style: Commas should always have a space after.
sw_input_soillayers <- data.frame(row.names = c(1,2,3,4,5,6))
                                                      ^
tests/testthat/test_ExtractData_Soils.R:88:57: style: Commas should always have a space after.
sw_input_soillayers <- data.frame(row.names = c(1,2,3,4,5,6))
                                                        ^
tests/testthat/test_ExtractData_Soils.R:88:59: style: Commas should always have a space after.
sw_input_soillayers <- data.frame(row.names = c(1,2,3,4,5,6))
                                                          ^
tests/testthat/test_ExtractData_Soils.R:92:4: style: Place a space before left parenthesis, except in a function call.
for(i in (3:22)){
   ^
tests/testthat/test_ExtractData_Soils.R:95:1: style: Lines should not be more than 80 characters.
sw_input_soillayers[,1] <- c("Site01","Site02","Site03","Site04","Site05","Site06")
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test_ExtractData_Soils.R:95:22: style: Commas should always have a space after.
sw_input_soillayers[,1] <- c("Site01","Site02","Site03","Site04","Site05","Site06")
                     ^
tests/testthat/test_ExtractData_Soils.R:95:39: style: Commas should always have a space after.
sw_input_soillayers[,1] <- c("Site01","Site02","Site03","Site04","Site05","Site06")
                                      ^
tests/testthat/test_ExtractData_Soils.R:95:48: style: Commas should always have a space after.
sw_input_soillayers[,1] <- c("Site01","Site02","Site03","Site04","Site05","Site06")
                                               ^
tests/testthat/test_ExtractData_Soils.R:95:57: style: Commas should always have a space after.
sw_input_soillayers[,1] <- c("Site01","Site02","Site03","Site04","Site05","Site06")
                                                        ^
tests/testthat/test_ExtractData_Soils.R:95:66: style: Commas should always have a space after.
sw_input_soillayers[,1] <- c("Site01","Site02","Site03","Site04","Site05","Site06")
                                                                 ^
tests/testthat/test_ExtractData_Soils.R:95:75: style: Commas should always have a space after.
sw_input_soillayers[,1] <- c("Site01","Site02","Site03","Site04","Site05","Site06")
                                                                          ^
tests/testthat/test_ExtractData_Soils.R:96:4: style: Place a space before left parenthesis, except in a function call.
for(i in (2:22)){
   ^
tests/testthat/test_ExtractData_Soils.R:97:24: style: Commas should always have a space after.
  sw_input_soillayers[,i] <- NA
                       ^
tests/testthat/test_ExtractData_Soils.R:102:1: style: Lines should not be more than 80 characters.
input_soils <- list("Matricd_L", "GravelContent_L", "EvapCoeff_L", "Grass_TranspCoeff_L", "Shrub_TranspCoeff_L", "Tree_TranspCoeff_L", "Forb_TranspCoeff_L",
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test_ExtractData_Soils.R:103:1: style: Lines should not be more than 80 characters.
                                "TranspRegion_L", "Sand_L", "Clay_L", "TOC_GperKG_L", "Imperm_L", "SoilTemp_L")
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test_ExtractData_Soils.R:110:7: style: Place a space before left parenthesis, except in a function call.
    if(counter >= length(input_soils)) {
      ^
tests/testthat/test_ExtractData_Soils.R:120:46: style: Commas should always have a space after.
sw_input_soils <- data.frame(row.names = c(1,2,3,4,5,6))
                                             ^
tests/testthat/test_ExtractData_Soils.R:120:48: style: Commas should always have a space after.
sw_input_soils <- data.frame(row.names = c(1,2,3,4,5,6))
                                               ^
tests/testthat/test_ExtractData_Soils.R:120:50: style: Commas should always have a space after.
sw_input_soils <- data.frame(row.names = c(1,2,3,4,5,6))
                                                 ^
tests/testthat/test_ExtractData_Soils.R:120:52: style: Commas should always have a space after.
sw_input_soils <- data.frame(row.names = c(1,2,3,4,5,6))
                                                   ^
tests/testthat/test_ExtractData_Soils.R:120:54: style: Commas should always have a space after.
sw_input_soils <- data.frame(row.names = c(1,2,3,4,5,6))
                                                     ^
tests/testthat/test_ExtractData_Soils.R:121:17: style: Commas should always have a space after.
sw_input_soils[,1] <- c("Site01","Site02","Site03","Site04","Site05","Site06")
                ^
tests/testthat/test_ExtractData_Soils.R:121:34: style: Commas should always have a space after.
sw_input_soils[,1] <- c("Site01","Site02","Site03","Site04","Site05","Site06")
                                 ^
tests/testthat/test_ExtractData_Soils.R:121:43: style: Commas should always have a space after.
sw_input_soils[,1] <- c("Site01","Site02","Site03","Site04","Site05","Site06")
                                          ^
tests/testthat/test_ExtractData_Soils.R:121:52: style: Commas should always have a space after.
sw_input_soils[,1] <- c("Site01","Site02","Site03","Site04","Site05","Site06")
                                                   ^
tests/testthat/test_ExtractData_Soils.R:121:61: style: Commas should always have a space after.
sw_input_soils[,1] <- c("Site01","Site02","Site03","Site04","Site05","Site06")
                                                            ^
tests/testthat/test_ExtractData_Soils.R:121:70: style: Commas should always have a space after.
sw_input_soils[,1] <- c("Site01","Site02","Site03","Site04","Site05","Site06")
                                                                     ^
tests/testthat/test_ExtractData_Soils.R:122:4: style: Place a space before left parenthesis, except in a function call.
for(i in (2:281)){
   ^
tests/testthat/test_ExtractData_Soils.R:123:19: style: Commas should always have a space after.
  sw_input_soils[,i] <- NA
                  ^
tests/testthat/test_ExtractData_Soils.R:129:1: style: Lines should not be more than 80 characters.
                                           include_sources, how_determine_sources, sw_input_soillayers, sw_input_soils_use,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test_ExtractData_Soils.R:135:1: style: Lines should not be more than 80 characters.
  # create copies of the above variables and change them here for expanding testing,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test_ExtractData_Soils.R:136:1: style: Lines should not be more than 80 characters.
  # tests rely on the variables above and changing them here will keep them reliable
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test_ExtractData_Soils.R:138:1: style: Lines should not be more than 80 characters.
                                                      field_sources, sim_size, "SWRunInformation")
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test_ExtractData_Soils.R:142:1: style: Lines should not be more than 80 characters.
  # create copies of the above variables and change them here for expanding testing,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test_ExtractData_Soils.R:143:1: style: Lines should not be more than 80 characters.
  # tests rely on the variables above and changing them here will keep them reliable
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test_ExtractData_Soils.R:144:1: style: Lines should not be more than 80 characters.
  include_sources <- rSFSW2::get_datasource_includefield(SWRunInformation, include_sources,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test_ExtractData_Soils.R:184:1: style: Lines should not be more than 80 characters.
                             dir_ex_soil = "/media/natemccauslin/SOILWAT_DATA/GIS/Data/Soils",
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test_ExtractData_Soils.R:185:1: style: Lines should not be more than 80 characters.
                             fnames_in = fnames_in, resume, verbose, default_TOC_GperKG = 0)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test_ExtractData_Soils.R:209:6: style: Place a space before left parenthesis, except in a function call.
  for(i in c(1, length(density_vals))){
     ^
tests/testthat/test_ExtractData_Soils.R:225:6: style: Place a space before left parenthesis, except in a function call.
  for(j in c(2, 6)){
     ^
tests/testthat/test_ExtractData_Soils.R:226:8: style: Place a space before left parenthesis, except in a function call.
    for(i in c(1, length(sand_vals))){
       ^
tests/testthat/test_ExtractData_Soils.R:227:9: style: Place a space before left parenthesis, except in a function call.
      if(!is.na(MMC[["data"]][todos, grep("sand", MMC[["cn"]])[j]][[i]])){
        ^
tests/testthat/test_ExtractData_Soils.R:228:1: style: Lines should not be more than 80 characters.
        expect_type(MMC[["data"]][todos, grep("clay", MMC[["cn"]])[j]][[i]], "double")
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test_ExtractData_Soils.R:229:1: style: Lines should not be more than 80 characters.
        expect_type(MMC[["data"]][todos, grep("density", MMC[["cn"]])[j]][[i]], "double")
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test_ExtractData_Soils.R:230:1: style: Lines should not be more than 80 characters.
        expect_type(MMC[["data"]][todos, grep("depth", MMC[["cn"]])[j]][[i]], "double")
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test_ExtractData_Soils.R:231:1: style: Lines should not be more than 80 characters.
        expect_type(MMC[["data"]][todos, grep("carbon", MMC[["cn"]])[j]][[i]], "double")
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test_ExtractData_Soils.R:232:1: style: Lines should not be more than 80 characters.
        expect_type(MMC[["data"]][todos, grep("rock", MMC[["cn"]])[j]][[i]], "double")
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

test_rSFSW2_StyleSpelllingPractices.R:84: failure: Package spell checks
length(misspelled) not identical to 0.
1/1 mismatches
[1] 10 - 0 == 10
CaitlinA commented 6 years ago

@dschlaep O - sorry. I didn't realize those tests were being skipped on the CI. A few questions though, as these lintr test are brand new ...

(1) How are you getting such a comprehensive read out of all the specific lines with formatting or style errors? I only see that when I run devtools::test on my local that the lintr tests have failed: ✖ | 0 1 2 | Code style, spell checks, and good package practices [4.6 s]

(2) Could you address issue #2 in the workflow_guidelines. I haven't thoroughly investigated lintr yet. Are there things we are checking for and things we are not, etc.?

dschlaep commented 6 years ago

@CaitlinA Re 1: Depends on where you run this command (RStudio, etc) and what you specified as testthat::default_reporter(). The output that I posted was generated by running in Terminal: Rscript -e 'devtools::test()'

Re 2: I don't know what to write about lintr besides saying that it formalizes the style recommendations from Hadley Wickham's R package book (that I commented on in the review) and refer to the .lintr file.

dschlaep commented 6 years ago

Re 1: Note also that the lintr and spell checker are skipped if on CRAN is set, e.g., RStudio may set it and running testthat::test_file(...) also likely has set it -- but it really depends on how you set it all up.

This is why I added to the README:

Different tests/checks are run under different settings depending on the environmental setting NOT_CRAN and whether or not integration tests (i.e., those that run TestPrj4) are executed in parallel or serial mode. Thus, for greatest coverage, run checks both with and without option --as-cran respectively argument cran of function devtools::check() -- on the command line and interactively.

CaitlinA commented 6 years ago

Thanks for the info. In regards to (2) - I suppose I have just seen throughout the code there are #nolintr comments, as well as files that the lintr tests are skipped on. What is determining when things (function names, etc.) are brought to lintr standards and some are not?

On Thu, Aug 23, 2018 at 4:46 PM, daniel notifications@github.com wrote:

Re 1: Note also that the lintr and spell checker are skipped if on CRAN is set, e.g., RStudio may set it and running testthat::test_file(...) also likely has set it -- but it really depends on how you set it all up.

This is why I added to the README:

Different tests/checks are run under different settings depending on the environmental setting NOT_CRAN and whether or not integration tests (i.e., those that run TestPrj4) are executed in parallel or serial mode. Thus, for greatest coverage, run checks both with and without option --as-cran respectively argument cran of function devtools::check() -- on the command line and interactively.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/DrylandEcology/rSFSW2/pull/323#issuecomment-415607588, or mute the thread https://github.com/notifications/unsubscribe-auth/AM0e_7ANH3UR--bLftohJXLeuWf3BxFHks5uTz7qgaJpZM4VxAfO .

-- Caitlin Andrews Ecologist Southwest Biological Science Center U.S. Geological Survey Mobile: 802-922-3494 Office: 928-556-7036

dschlaep commented 6 years ago

See ?lintr::exclude

The goal is complete linting coverage.

-Daniel

On Aug 23, 2018, at 20:13, CaitlinA notifications@github.com wrote:

Thanks for the info. In regards to (2) - I suppose I have just seen throughout the code there are #nolintr comments, as well as files that the lintr tests are skipped on. What is determining when things (function names, etc.) are brought to lintr standards and some are not?

On Thu, Aug 23, 2018 at 4:46 PM, daniel notifications@github.com wrote:

Re 1: Note also that the lintr and spell checker are skipped if on CRAN is set, e.g., RStudio may set it and running testthat::test_file(...) also likely has set it -- but it really depends on how you set it all up.

This is why I added to the README:

Different tests/checks are run under different settings depending on the environmental setting NOT_CRAN and whether or not integration tests (i.e., those that run TestPrj4) are executed in parallel or serial mode. Thus, for greatest coverage, run checks both with and without option --as-cran respectively argument cran of function devtools::check() -- on the command line and interactively.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/DrylandEcology/rSFSW2/pull/323#issuecomment-415607588, or mute the thread https://github.com/notifications/unsubscribe-auth/AM0e_7ANH3UR--bLftohJXLeuWf3BxFHks5uTz7qgaJpZM4VxAfO .

-- Caitlin Andrews Ecologist Southwest Biological Science Center U.S. Geological Survey Mobile: 802-922-3494 Office: 928-556-7036 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/DrylandEcology/rSFSW2/pull/323#issuecomment-415611696, or mute the thread https://github.com/notifications/unsubscribe-auth/AEAp2-_lbHhFh1RZ0N0h24j27ihbCsYYks5uT0U8gaJpZM4VxAfO.

dschlaep commented 6 years ago

I will update the README section on preparing a pull-request with specific instructions so that the chances are maximized that all relevant checks are run.

dschlaep commented 6 years ago

@CaitlinA

I updated the README (https://github.com/DrylandEcology/rSFSW2/tree/expand_README).

Please comment on the suggested changes: https://github.com/DrylandEcology/rSFSW2/compare/Enhancement%23305_GriddedGlobalDatav5...expand_README particularly in the How to contribute section

CaitlinA commented 6 years ago

@nip5

I am still failing one formatting/lintr test on my local:

`✖ | 1 1 1 | Code style, spell checks, and good package practices [5.3 s] ──────────────────────────────────────────────────────────────────────────────────────────── test_rSFSW2_StyleSpelllingPractices.R:29: error: Package code style object 'T_and_F_symbol_linter' not found 1: lintr::expect_lint_free(exclusions = as.list(normalizePath(file.path("..", "..", "R", files_not_tolint)))) at /Users/candrews/Documents/Git/rSFSW2/tests/testthat/test_rSFSW2_StyleSpelllingPractices.R:29 2: lint_package(...) 3: read_settings(path) 4: get_setting(setting, config, default_settings) 5: eval(parse(text = config[[setting]])) 6: eval(parse(text = config[[setting]])) 7: with_defaults(line_length_linter(80L), object_name_linter = NULL, camel_case_linter = NULL,snake_case_linter = NULL, T_and_F_symbol_linter, nonportable_path_linter, semicolon_terminator_linter, seq_linter, undesirable_function_linter, undesirable_operator_linter, unneeded_concatenation_linter)

test_rSFSW2_StyleSpelllingPractices.R:91: skip: Package good practices`

Do you see this locally?

dschlaep commented 6 years ago

@CaitlinA The error message object 'T_and_F_symbol_linter' not found sounds to me as if the lintr version on your system doesn't know this linter. It was introduced with commit https://github.com/jimhester/lintr/commit/12d3edbf282288fa8604828c4bfb1946b908e415#diff-d8545f582f6941364f7c41d652251cda on March 6, 2017. Maybe need to install latest version from github?

nip5 commented 6 years ago

@dschlaep That's correct, I had the same issue until I updated from1.0.2 to 1.0.2.9000.

nip5 commented 6 years ago

Some new lintr tests will fail because of commit b28734c but all tests pass for my function.