Clean the code in create_codemeta.R #207
Conversation
Assign the result of the if()-statement instead of assigning to the same variable in all branches
This decreases the level of nesting and the number of closing curly braces at the end...
Within this function, use the better name "codemeta" instead of "cm"
- name this function that is called first set_relatedLink_1() - rename set_relatedLink() that is called second to set_relatedLink_2()
Do not call get_file() twice.
Put get_url_*_package() functions into new file get_url.R
- instead of format-strings for sprintf(), assign these functions for CRAN and BioConductor in the lookup vector "url_generators" - for GitHub, use new function get_url_github_account() and do not use the entry "_GITHUB_" any more.
- create new functions get_url_bioconductor_package_2(), get_url_cran_package_2() - let get_url_bioconductor_package() use get_url_bioconductor_package_2() in order to avoid duplication
- now that functions instead of strings are to be looked up, a list is required instead of a named vector. - R CMD Check was failing but now succeeds
- define mapping between base-URL patterns and functions generating full URLs in list "url_generators" - do not assign to variable "id", the function returns what the if-else-statement returns - swap if/else branch so that NULL remains for the else branch which is the default anyway - if dep$provider is given, try to find a pattern in the names of "url_generators" and apply the corresponding function get_url_*_package_2() to create the full URL
- return early for the special case of dep$name == "R" - this simplifies the if-else-construct
|
|
||
| ## no cm provided, but codemeta.json found in pkg | ||
| if (file.exists(get_file("codemeta.json", root))) { | ||
| cm <- if (file.exists(json_file)) { | ||
|
|
There was a problem hiding this comment.
I'd prefer the assignment to be inside the in and else, not outside of them. :-)
There was a problem hiding this comment.
In general or only in this case? In general, I prefer to do the assignment outside, because to me variable <- is already a duplication when used in both, the if and the else branch. There is a risk that you use different variable names, such as in
if (condition) {
my_long_variable_name <- "a"
} else {
my_long_Variable_name <- "b"
}
To me, the following looks more clean and is less error-prone:
my_long_variable_name <- if (condition) {
"a"
} else {
"b"
}
However, a prerequisite is that the bodies of the if and else branches are small, in the best case one line only...
So, do you want me to undo this change? I have already used the "outside assignment" at many other places in the code...
There was a problem hiding this comment.
Another advantage: when doing the assignment outside, the value of NULL is automatically assigned to the variable if there is no else branch, just as stated in help("if"):
if returns the value of the expression evaluated, or NULL invisibly if none was (which may happen if there is no else).
So, instead of
if (condition) {
result <- "a"
} else {
result <- NULL
}
(a construct that is used a lot in the existing code), you can simply write
result <- if (condition) {
"a"
} # otherwise NULL implicitly
Well, this might not be obvious to everyone and maybe we should prefer
result <- if (condition) {
"a"
} else {
NULL
}
|
You don't plan any more changes and I can merge, right? |
|
Right, I am awaiting the merge impatiently and then feel safer to continue... |
|
Thanks a ton! Did you get my email? |
## 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.
Please consider my suggestions for cleaner code. I extracted parts of create_codemeta() into sub-functions, avoiding too deep nesting of if-else-statements. Now, there are only three closing curly braces (still too many) at the end of the function. In the original version there were eight! See my single commits for details. With the last commits I came back to parse_depends.R where I reused and continued the concepts that have resulted from previous cleaning steps. Looking forward to hearing your opinion! Kind regards, Hauke