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

Clean code in guess_github_metadata.R #216

Merged
merged 9 commits into from Jan 31, 2019
Merged

Conversation

hsonne
Copy link
Contributor

@hsonne hsonne commented Dec 11, 2018

Please consider my attempt to clean the code in guess_github_metadata.R. See my commit messages for details. Please review carefully since the behaviour changed slightly, but hopefully improved.
Thanks for consideration, Hauke

cboettig and others added 9 commits December 4, 2018 09:13
- This version (using "ceiling = 0") is never used as
  the version in utils.R seems to take precedence
  (because "utils" comes after "guess_github_metadata"
  in alphabetical order?)
- What is the desired behaviour, ceiling = 0 or the
  default (ceiling = NULL), or should ceiling be an
  argument to uses_git()?
- Return early with NULL if git is not used
- Consequently use the pipe operator. It makes the code
  look clean but is there maybe any reason not to do so?
- I think that there is no need to call
  git2r::repository() since it is already called
  within guess_github()
- Return early
- Use paste(..., sep = "/") instead of paste0()
- Use the pipe operator to avoid repetition of
  variable name "github"
- Use getElment(1) instead of "[[1]]" to make this
  step a bit more obvious
- no need for intermediate variables "owner" and
  "repo" since they are used only once in the call
   to gh::gh(). There, their meaning gets clear
   because the arguments are set by name.
- Set silent = TRUE first in call to gh::gh(). This
  allows for a cleaner indentation.
@maelle
Copy link
Member

maelle commented Dec 11, 2018

Thank you! Can the slight changes in behaviour be described in tests, potentially with mocking?

@maelle
Copy link
Member

maelle commented Dec 18, 2018

@hsonne could you answer the question above, or write a summary of the changes in behavior? Thank you!

@maelle
Copy link
Member

maelle commented Jan 14, 2019

@hsonne Happy New Year! Any feedback on the above? Thank you!

@hsonne
Copy link
Contributor Author

hsonne commented Jan 14, 2019

Hi @maelle and happy New Year to you, too. Sorry for not having replied so far.
The difference is in github_path() only. In the old version, git2r::repository() is called and the result is stored in r but then, r is not used any further. That's why in my version I do not call this function any more. Also, in my version NULL is returned early if guess_github() already returned NULL.
I am hesitating because I do not know if this is the intended bahaviour. Maybe there was a reason to call git2r::repository()? Is it just called to let the function check if the repository exists? I would like to discuss the intended behaviour before committing my change. I could also undo this change so that you can merge the others.

@maelle
Copy link
Member

maelle commented Jan 16, 2019

@cboettig do you remember the reason for the use of git2r::repository()?

@cboettig
Copy link
Member

I believe it is used to guess the repository URL (if it's not given in the DESCRIPTION), see:

guess_github <- function(root = ".") {
## from devtools
# https://github.com/r-lib/devtools/blob/21fe55a912ca4eaa49ef5b7d891ff3e2aae7a370/R/git.R#L130
# GPL>=2 code
if (uses_git(root)) {
r <- git2r::repository(root, discover = TRUE)
r_remote_urls <- grep("github", remote_urls(r), value = TRUE)
out <- r_remote_urls[[1]]
gsub("\\.git$", "", gsub("git@github.com:", "https://github.com/", out))
} else {

@cboettig
Copy link
Member

@maelle Just wanted check in and see where we are on this? is this good to merge now?

@maelle
Copy link
Member

maelle commented Jan 31, 2019

It depends on your decision on #216 (comment) IIRC

@cboettig
Copy link
Member

@hsonne @maelle Thanks! Yes, this looks good to me. I believe the unused call of git2r::repository was left over from refactoring.

@cboettig cboettig merged commit 4d9a4f0 into ropensci:dev Jan 31, 2019
maelle added a commit that referenced this pull request Apr 1, 2020
## Deprecation

* The use_git_hook argument of write_codemeta() has been deprecated. Solutions for keeping DESCRIPTION and codemeta.json in sync are available in the docs.

## Enhancements

* Docs were improved to make a better case for codemetar.

* Changes in the way codeRepository is guessed. codemetar can now recognize an URL from GitHub, GitLab, Bitbucket, R-Forge among several URLs in DESCRIPTION, to assign it to codeRepository. If no URL in DESCRIPTION is from any of these providers, `guess_github()` is called.

* Adds documentation of internet needs and verbosity to steps downloading information from the web (#270, @Bisaloo)

* New argument `write_minimeta` for `write_codemeta()` indicating whether to also create the file schemaorg.json that  corresponds to the metadata Google would validate, to be inserted to a webpage for SEO. It is saved as "schemaorg.json" alongside `path` (by default, "codemeta.json"). This functionality requires the `jsonld` package (listed under `Suggests`).

## Bug fixes

* Fix for detecting rOpenSci review badge (@sckott, #236)

* Fix extraction of ORCID when composite comment (@billy34, #231)

* Fix bug in crosswalking (#243)

* Bug fix: the codeRepository is updated if there's any URL in DESCRIPTION.

* Bug fix: the README information is now updated by codemeta_readme(). Previously if e.g. a developmentStatus had been set previously, it was never updated.

## Internals

* Code cleaning following the book Martin, Robert C. Clean code: a handbook of agile software craftsmanship. Pearson Education, 2009. (@hsonne, #201, #202, #204, #205, #206, #207, #209, #210, #211, #212, #216, #218, #219, #220, #221).

* Use of re-usable Rmd pieces for the README, intro vignette and man pages to reduce copy-pasting.
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

3 participants