cynkra / fledge

Wings for your R packages: Streamline the process of versioning R packages and updating NEWS
https://fledge.cynkra.com
186 stars 11 forks source link

get_main_branch_config() reliance on length(tablename) fails with 0 row table #767

Closed D3SL closed 4 weeks ago

D3SL commented 2 months ago

I just found this package and was very excited, unfortunately I haven't been able to get it to work yet. I've troubleshot a few of the stumbling blocks to getting fledge::bump_version() to work but this one seems to be impassable.

In bump-version.R get_main_branch_config() (line 98) relies on the test length(local) (line 103). Dataframes, tibbles, and data.tables all return their number of columns for length(), even if they are empty. This causes get_main_branch_config() to spuriously return init[init$level == "local", ]$value (line 104) instead of init[init$level == "global"]$value (line 108)

Below is a redacted sample of my exact data to demonstrate


# object 'config' as would be retrieved by gert::git_config(repo)
config<-structure(list(name = c("diff.astextplain.textconv", "filter.lfs.clean", 
"filter.lfs.smudge", "filter.lfs.process", "filter.lfs.required", 
"http.sslbackend", "http.sslcainfo", "core.autocrlf", "core.fscache", 
"core.symlinks", "pull.rebase", "credential.helper", "credential.https://dev.azure.com.usehttppath", 
"core.editor", "core.excludesfile", "gui.recentrepo", "user.email", 
"user.name", "init.defaultbranch", "core.repositoryformatversion", 
"core.filemode", "core.bare", "core.logallrefupdates", "core.symlinks", 
"core.ignorecase", "remote.origin.url", "remote.origin.fetch", 
"branch.main.remote", "branch.main.merge"), value = c("astextplain", 
"git-lfs clean -- %f", "git-lfs smudge -- %f", "git-lfs filter-process", 
"true", "openssl", "C:/Program Files/Git/mingw64/ssl/certs/ca-bundle.crt", 
"true", "true", "false", "false", "manager-core", "true", "\"C:\\Users\\Foo\\AppData\\Local\\Programs\\Microsoft VS Code\\Code.exe\" --wait", 
"C:/Users/Foo/.gitignore", "C:/foo/bar/baz/foobar", 
"foo@bar.baz", "Foo Bar", "main", "0", "false", 
"false", "true", "false", "true", "http://giteahere:3000/Foo/Bar.git", 
"+refs/heads/*:refs/remotes/origin/*", "origin", "refs/heads/main"
), level = c("system", "system", "system", "system", "system", 
"system", "system", "system", "system", "system", "system", "system", 
"system", "global", "global", "global", "global", "global", "global", 
"local", "local", "local", "local", "local", "local", "local", 
"local", "local", "local")), row.names = c(NA, 29L), class = c("tbl_df", 
"tbl", "data.frame"))

# contents of get_main_branch_config() configured to run interactively
init <- config[config$name == "init.defaultbranch", ]
print(init)

# A tibble: 1 × 3
#  name               value  level 
#  <chr>              <chr>  <chr> 
#1 init.defaultbranch main global

local <- init[init$level == "local", ] 
print(local)
# A tibble: 0 × 3
# ℹ 3 variables: name <chr>, value <chr>, level <chr>

if (length(local)) {  # this test fails because length(local)==3, _nrow(local)_==0
  print(local$value)    # the function prematurely ends, returning an empty table here
} else {
  global <- init[init$level == "global"] # this is missing a comma and would also break
  print(global$value)
}

If you want to stick with base R this is what I would suggest:

  get_main_branch_config <- function(repo) {
  #retrieve config of repo, filter down to init.defaultbranch values
  config <- gert::git_config(repo)
  init <- config[config$name == "init.defaultbranch", ]

  #return local default branch if it exists, otherwise default to global
  if("local" %in% init$level){
    return(init[init$level == "local",]$value)
  } else { 
    return(init[init$level == "global",]$value)
  }
}

You could also just get rid of this function entirely.

get_main_branch <- function(repo = getwd()) {
  remote <- "origin"            #hardcoded "origin" remote name 
  remote_list <- gert::git_remote_list(repo)
  if (remote %in% remote_list$name) {
    remote_main <- get_main_branch_remote(remote, repo)
    if (length(remote_main)) {
      return(remote_main)
    }
  } else{ 
    init<-gert::git_config(getwd())[config$name == "init.defaultbranch",]
    if("local" %in% init$level){
      return(init[init$level == "local",]$value)
    } else { 
      return(init[init$level == "global",]$value)
    }
  }
}
maelle commented 2 months ago

Thank you!! Would you like to make a PR with what comes after "If you want to stick with base R this is what I would suggest:"?

D3SL commented 2 months ago

Done in #775 , I think. I've never actually made a pull request to a third party repo before so I'm not 100% sure I did that right.