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

added binary repo path in initRepo, also support "account_name/repo_name" format as input… #73

Closed
wants to merge 4 commits into from

Conversation

dracodoc
Copy link

  • create binary repo path.
    The purpose is that with a drat repo added to repo options, install_packages from CRAN often check the drat repo for binary package and report warnings. With empty index created for major windows/mac versions, there will not be warnings.
    Of course it'll also help if user want to add binary package later.
  • support input format account_name/repo_name.
    This is common usage so users should be familiar with it. With account name specified, the git remote will be added to the local repo. If push = TRUE it can also push to the github remote (if ssh-agent is available).

My plan is to make the drat repo creation process as smooth as possible. Ideally user should be able to do this:

  1. create a repo in github.
  2. run drat::initRepo("account_name/repo_name", basepath).
  3. run another function to build package, insert package, push to github remote.

For every package build, user should only need run one command in 3 to update the repo.

@dracodoc
Copy link
Author

Customized branch name is added to initRepo. I also make it a parameter in insertPackage for easier one time use. See #61

@dracodoc dracodoc mentioned this pull request Mar 12, 2018
…hub account name.

- Added notes of getOptions for user to check the result. This may be obvious for some advanced users but not so for some users.
@dracodoc
Copy link
Author

I modified the help of addRepo a little bit.

Note my PR may include some redundant diffs because I used a different version of roxygen. Hopefully this will not create too much hassle for you.

@eddelbuettel
Copy link
Owner

I really don't like being overwhelmed by multiple PRs I din't ask for. You did not motivate WHAT you where going to do and HOW. You are kind enough to prepare a PR but now I have to check seven files some of which seem unrelated.

This is not a good workflow.

@dracodoc
Copy link
Author

Sorry for the messed up workflow.

Some files are redundant because I used different version roxygen. I also should split commits for different purposes into different PRs.

Let me see if I can reorganize them.

@dracodoc dracodoc closed this Mar 12, 2018
@eddelbuettel
Copy link
Owner

Please. Slow. Down.

Don't throw an infinite number of posts at me. I just sat down and have not had time to read your first wave.

##' @author Dirk Eddelbuettel
initRepo <- function(name="drat", basepath="~/git") {

initRepo <- function(name="drat", basepath="~/git",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of making all the parts individual arguments and pasteing them together, why not just have the user pass in a url?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's easy to just copy username/repo from github url, and that should be enough for a lot of user cases. In most cases the branch doesn't need to changed from default too.

mac_r_versions <- list(mavericks = c("3.1", "3.2", "3.3"),
contrib = c("3.0", "3.1", "3.2"),
el_capitan = c("3.4", "3.5"))
mac_binary_folders <- lapply(names(mac_binary_bases),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use mapply here

@@ -81,7 +84,7 @@ insertPackage <- function(file,
commit <- TRUE
}

branch <- getOption("dratBranch", "gh-pages")
# branch <- getOption("dratBranch", "gh-pages")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't leave dead code around w/o a good reason.

@dracodoc
Copy link
Author

@nfultz ,thanks for the review, they are very helpful.

I probably will create new commits to separate different focus points and will not use this PR, so maybe you can check the new commits later.

" with repositories paths"))
# push repo
if (push) git2r::push(repo, name = "origin",
refspec = paste0("refs/heads/", branch))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also note there have been issues with git2r::push to github, please check if this works with a stock cran install before merging in. - ropensci/git2r#331

@eddelbuettel
Copy link
Owner

@nfultz Thanks for your patience and comments. The changes by @dracodoc were a little premature and hasty; a little time and reflection may improve them. If not, I doubt I'll merge.

@dracodoc To make things very plain: I am the maintainer. So whenever something changes (or worse, breaks) people come to me and I have to deal with. You may desire dozens of changes; maybe those are best kept in a local fork of yours until they mature. We do take code contributions, and they have made drat better. But please let's not repeat the bad experience of today. Don't bulk submit. Quantity is not a substitute for quality. Lastly: I do this in public. You see a name, and a face. I tend to dislike contributions from pseudonyms and anonymous users. If you want to be take seriously, show a name. See files ChangeLog and inst/NEWS.Rd. Thanks.

@dracodoc
Copy link
Author

dracodoc commented Mar 13, 2018

@eddelbuettel
My intention is to make some default usage of drat to be as smooth as possible.

It's not really about "my desire", actually I'm not even using drat for my package now, because I need to run several commands after every update. I plan to release packages in drat repo only after I can automate the build package/update repo process as much as possible.

I think whether to show full name and face in github profile is up to personal reference and there is nothing to discuss about it. I didn't expect this to be a requirement for contribution.

@dracodoc
Copy link
Author

dracodoc commented Mar 13, 2018

I've made a separate branch to focus on support customized branch here.

I'm gonna finish what I have in plan in a better shape, reference them here just for sake of completion.

@dracodoc
Copy link
Author

dracodoc commented Mar 13, 2018

Another branch focused on github_user_name/repo_name type of input. git remote will be added to the repo.

Binary repo paths and empty package index are created. Previously with a drat repo added to repo options, install_packages from CRAN often check the drat repo for binary package and report warnings. With empty index created for major windows/mac versions, there will not be warnings.

@nfultz I considered your comments carefully:

  • I agree an url can have all the information for user_name, repo_name, remote_type. However I was planning to use the github_user_name/repo_name format in several places. Use the full url in those places seemed to be a little bit heavy weight, especially when most of time only github_user_name/repo_name is needed.
  • for mapply comment, I tried but not sure why that will help. Do you mean with simplify = TRUE it can save the unlist step later? I didn't find that to be the case.
  • I removed the push action since it may not be reliable, and it's better left to user to push manually or via their own script.

@dracodoc
Copy link
Author

dracodoc commented Mar 13, 2018

There is another point in my plan, which is to build the package, insert package, update the repo.

The idea is that user can make drat repo with minimal effort(I'm repeating myself because this was never discussed):

  1. create a repo in github.
  2. run drat::initRepo("account_name/repo_name", basepath).
  3. run another function to build package, insert package, push to github remote.

For every package build, user should only need run step 3 once to update the repo.

Step 3 involves using devtools::build in my script, but drat don't need that heavy dependencies so I'll not make step 3 into a function. User should be able to make his/her own script for step 3 easily.

For my usage I just make a custom action in source tree so I can do several things with one click:

  • update master branch to my development branch, this will release the package to users via remote::install_github
  • build the package and dependency package that need to use development version, update drat repo, so they are also available through drat.

Previously I also had plan to edit vignettes but that probably is not needed.

@dracodoc
Copy link
Author

dracodoc commented Mar 13, 2018

Besides, my initial commit was mostly about binary repo paths, which was discussed in #46 2 years ago, asked by me again in #71, and I think @eddelbuettel asked me to create a PR for it.

My biggest mistake is that I worked through several issues in same branch and all commits show up together in same PR in a short time, that was too overwhelming.

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