Closed ThierryO closed 3 years ago
Thanks for the feedback
- concerning the added docker test
This test runs only on hub.docker.com and tests if checklist
is correctly installed in its Dockerimage.
concerning
new_branch()
- it is a somewhat aberrant function in checklist as it is not related to checks (it would actually better fit in git2r)
Yes, you could have it in other packages too. However git2r
prefers to only translate single git commands.
Some people have a hard time starting a new branch from an updated main branch. This functions attempt to make their life easier. A side effect is that is make my life easier too ;-)
* when including other git related functions, you may want to group them and provide specific documentation (a dedicated vignette), and also bring it to attention in our git-course tutorial repo
good idea
* another option is to start a separate inbogit package with easy-to-use git functionality (if you have more functions in mind)
no more git stuff ideas at the moment
- when running it, I got the following error:
> checklist::new_branch("test-mag-weg") [updated] 830886ac01..0000000000 refs/remotes/origin/afwijkendeCurves [updated] 051ac138b7..0000000000 refs/remotes/origin/handleiding Error in push(repo, "origin", sprintf("refs/heads/%s", branch), set_upstream = TRUE) : Error in 'git2r_push': Unable to authenticate with supplied credentials In addition: Warning message: `afwijkendeCurves` diverged from the main origin branch.
https or ssh remote? IMHO we should all switch to ssh by default. Maybe we should return an error when the remote is not ssh.
check_package()
indeed runsbuilt_site()
in interactive sessions, but I noticed a depreciation note pops up... Admittedly, after runningcheck_package()
interactively after not having done it for some checklist versions, I noticed it has become an active process which opens some windows, for instance the Markers tab if linters are found and a browser with the pkgdown website... And this made me wondering what a user prefers: all kind of things popping up, or a written report? I tested to write the output ofcheck_package()
to a variable, hoping nothing would pop up, but it still did. Using the argumentspkgdown = FALSE
andquiet = TRUE
helped me to do so, but why is the pkgdown website still popping up when (only) usingquiet = TRUE
? I suggest not showing it whenquiet = TRUE
, unless the user explicitly combines it withpkgdown = TRUE
. (So defaultpkgdown
tointeractive() & !quiet
.)
What deprecated note did you get?
I'll change the code to that you don't get the pkgdown preview when quiet == FALSE
. You really want the lintr markers tab, as that is the easiest way to locate the linters.
What deprecated note did you get?
WARNING] Deprecated: markdown_github. Use gfm instead.
I'll change the code to that you don't get the pkgdown preview when
quiet == FALSE
.
Eh, when quiet == TRUE
, but you've done it the right way in the commit. ;-)
https or ssh remote? IMHO we should all switch to ssh by default. Maybe we should return an error when the remote is not ssh.
ssh. Ok to use it as a default, but it is anyhow a good idea to return an informative error when another protocol is used. Unless you want to answer a lot of questions on it... ;-)
Does warnings stem from pkgdown. I've been ignoring them for now.
Do you use an ssh key with or without password?
Does warnings stem from pkgdown. I've been ignoring them for now.
Yes, I suppose so...
Do you use an ssh key with or without password?
Oups, correction: I used https yesterday... (Concerning ssh: I use it without password, and this works when using push or pull from the git2r package. I could have a try with your package, if I only knew which of my repo's use a ssh connection...)
Switching for https to ssh is straightforward and reversable.
git remote remove origin
git remote add origin <insert ssh url you copy from github>
After following the above instructions to switch to ssh and using checklist::new_branch()
, I get the following error:
> checklist::new_branch("test-mag-weg")
Error in fetch(repo, "origin", verbose = verbose) :
Error in 'git2r_remote_fetch': error authenticating: failed connecting agent
So either it doesn't work on ssh, or the switch to ssh failed...
Yes, he is active, and I added the key now. Afterwards I pulled some changes and pushed to make sure the upstream was connected again, and the connection was clearly fine. But I still get this error...
Deprecated: markdown_github. Use gfm
was fixed in the development version of pkgdown. See https://github.com/r-lib/pkgdown/issues/1618
see suggestion in PR #56
Honestly I just had a look at the code and tested it in one package repo, I did no full tests in different situations:
check_package()
and I got no additional warning (checklist is installed on my computer but was not loaded)new_branch()
The branch was added locally, but not on the origin, and the push and pull buttons were disabled after this action. (I tested this on a repo that is connected to the origin using https, from experience I know that functions
push()
andpull()
from the git2r package only work properly using a ssh connection...) Because of this major concern that the regular setup of github repo's at INBO needs to be adapted for this function to work properly, I did not yet test the functionality ofnew_branch()
into detail. It seems better to me to wait until I know how you will deal with it (or just till I have some more time to do it properly). ;-)check_package()
indeed runsbuilt_site()
in interactive sessions, but I noticed a depreciation note pops up... Admittedly, after runningcheck_package()
interactively after not having done it for some checklist versions, I noticed it has become an active process which opens some windows, for instance the Markers tab if linters are found and a browser with the pkgdown website... And this made me wondering what a user prefers: all kind of things popping up, or a written report? I tested to write the output ofcheck_package()
to a variable, hoping nothing would pop up, but it still did. Using the argumentspkgdown = FALSE
andquiet = TRUE
helped me to do so, but why is the pkgdown website still popping up when (only) usingquiet = TRUE
? I suggest not showing it whenquiet = TRUE
, unless the user explicitly combines it withpkgdown = TRUE
. (So defaultpkgdown
tointeractive() & !quiet
.)