inbo / checklist

An R package for checking R packages and R code
https://inbo.github.io/checklist
GNU General Public License v3.0
16 stars 2 forks source link

Ssh vignette #67

Closed hansvancalster closed 2 years ago

hansvancalster commented 3 years ago

You might want to merge this into another branch...

hansvancalster commented 3 years ago

check_package check fails in GHA (complains about missing documentation), but locally everything seems OK (locally devtools::document() does not change anything)

florisvdh commented 3 years ago

Interesting @hansvancalster. I didn't look in detail but it seems that most of this is not specific to checklist, am I right? E.g. the same thing is needed to compile an Rmarkdown file that calls git2r directly.

In that case it may be better to add it as a general tutorial on the tutorials website. I suspect it would even solve https://github.com/inbo/tutorials/issues/26, if I understand that issue right.

That's just a suggestion, and even if it would indeed move to tutorials it would still be a smart thing to mention the need here.

hansvancalster commented 3 years ago

@florisvdh I wouldn't place it in tutorials (although it is indeed generic). The reason is that tutorials should reflect best practice and best in class packages. I believe we should move from git2r to gert for that reason. The developers of that package have gone to great lengths to resolve problems of authentication with Git / GitHub on different OS.

In the longer run, it would probably also be best for the checklist package to switch from git2r to gert - but as @ThierryO pointed out, this is indeed a major change.

A tutorial on passwords in GIT is still needed, but it would rather promote the HTTPS protocol instead of the SSH protocol (as tidyverse developers (especially packages gert, gh, usethis) and GitHub do. I wrote the vignette in this PR as a service to users who need to switch to the SSH protocol for some checklist functions that currently use git2r under the hood.

For an in-depth discussion of this, you can read the blog post when usethis released version 2.0.0 (which was mainly the switch from git2r to gert).

ElsLommelen commented 3 years ago

but it would rather promote the HTTPS protocol instead of the SSH protocol (as tidyverse developers (especially packages gert, gh, usethis) and GitHub do.

I thought @ThierryO rather wanted to promote SSH instead of HTTPS at INBO? See here in the last half of the post.

Anyway, I also planned on writing a similar vignette for package forrescalc (which also uses git2r), so I am in favour of @florisvdh 's idea to make it a general tutorial which could be linked to from the packages where needed. (And I am very happy to see the job is done for me, apparently postponing can have advantages. ;-) ) Independent of the discussion on which protocol we should promote at INBO, I think it could be useful to have a reference to both protocols in our tutorials and a short explanation of the difference (e.g. advantages and disadvantages). Probably at least some users will for some reason need the other protocol than the one that is promoted, and it is nice to find an easy and trusted reference to a manual in this case.

By the way, thinking of tutorials and r_universe: wouldn't it be good to add the tutorials as articles at INBO r_universe? Apparently r_universe is very good to bundle articles, and it seems nice if we can leaf/search through all vignettes and tutorials there. And the SSH manual would certainly fit there: close to the packages, easy to refer to from the packages, and accessible as a manual for any user of INBO r_universe. (While checklist is really for the advanced R user that makes his/her own packages, also starting R users may need the SSH manual, but the latter are very unlikely to know the checklist package at all...)

ElsLommelen commented 3 years ago

check_package check fails in GHA (complains about missing documentation), but locally everything seems OK (locally devtools::document() does not change anything)

@hansvancalster This seems a generic problem when installing checklist during CI. I solved it for one of my packages by adding remotes::install_cran("codemetar") before checklist is installed. I hope this helps? (It would probably be better to also fix this in the templates of checklist to solve it immediately for all packages that use checklist.)

florisvdh commented 3 years ago

@ElsLommelen @hansvancalster thanks for sharing these useful thoughts (and for the link to the usethis blog). Let's await how Thierry sees this.

In case the vignette stays here, do note that all vignettes will still end up in the tutorials website, with a clear prefix à la 'package: title' (something @LienReyserhove will probably work on this year). Of course it's not the same as a standalone tutorial since the contents would not specifically tie to checklist in the latter case.

By the way, thinking of tutorials and r_universe: wouldn't it be good to add the tutorials as articles at INBO r_universe?

@ElsLommelen I can't judge because of limited background - you have looked better at it - anyhow this sounds good! I suggest you add your suggestion as an issue in the tutorials repo to gather further feedback and continue the discussion there.

ElsLommelen commented 3 years ago
  • you have looked better at it -

Eh, @florisvdh , it looks nice in the keynote of UseR, but I must admit I haven't tested it yet in practice...

hansvancalster commented 2 years ago

@florisvdh @ElsLommelen @ThierryO I have a PR #68 ready to switch from git2r to gert. If that PR gets merged, than this PR #67 can be closed and the vignette can become part of a more general INBO tutorial explaining both the HTTPS and SSH protocols.

ThierryO commented 2 years ago

check_package check fails in GHA (complains about missing documentation), but locally everything seems OK (locally devtools::document() does not change anything)

You probably had a different version of Roxgen2 installed on your machine. That version number is stored in the DESCRIPTION. check_document() checks is any file is changed after updating the document.

ThierryO commented 2 years ago

If you know where to look you find it in the log file. I've tried to improve the error message in ef53c5ac

hansvancalster commented 2 years ago

If you know where to look you find it in the log file. I've tried to improve the error message in ef53c5a

Good idea to improve the error message in that way!

hansvancalster commented 2 years ago

Now #68 is merged, this PR can be closed.