Orcid

[maelle]

Cf #63

[maelle]

Not adding my ORCID would be weird 😹

[codecov-io]

Codecov Report

Merging #70 into master will increase coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #70      +/-   ##
==========================================
+ Coverage   98.77%   98.82%   +0.05%     
==========================================
  Files          18       18              
  Lines        1065     1111      +46     
==========================================
+ Hits         1052     1098      +46     
  Misses         13       13

Impacted Files	Coverage Δ
R/description.R	97.6% <100%> (+0.09%)	⬆️
R/authors-at-r.R	98.78% <100%> (+0.34%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9013f7d...6d5caa4. Read the comment docs.

[gaborcsardi]

So, what happens if there is already an orcid there?

[maelle]

Right. Do you agree that the old one should get replaced by the new one if that's the case?

[gaborcsardi]

Yes, I do. It would be nice to have a test case for this as well. Thanks!

[maelle]

I'll also add a test for this.

[gaborcsardi]

Can you pls rewrite this more in the style of http://style.tidyverse.org/news.html#before-release

New add_orcid() method and desc_add_orcid() functions...

[maelle]

I tried :-)

[gaborcsardi]

Thanks! Looks great in general, just had a few questions, and some small requests.

[maelle]

Thanks, will make the changes. I was also wondering whether/how to add checks that the ORCIDs are unique i.e. that the DESCRIPTION doesn't feature several authors with the same ORCID. At which step could this be checked? Are there any other similar checks already (e.g. that there's at least one maintainer) <- I could probably go read more code to answer that question.

[gaborcsardi]

No author checks currently. R CMD check checks for the single maintainer, maybe R CMD build as well, because it creates the Maintainer field. Not a big priority for us, I would say. Nevertheless the placeholder is here:

desc/R/syntax_checks.R

Line 165 in 9013f7d

check_field.DescriptionAuthorsAtR <- function(x, warn = FALSE, ...) {

[maelle]

Ok! But does it make sense for add_orcid to error if the arguments provided return more than one author since it'd mean one adds the same ORCID to several authors?

[gaborcsardi]

Yes, I think that would make sense indeed. Good catch!

[maelle]

I'm also going to add a mocking test for ORCID_ID.

[maelle]

Thanks for the feedback, I've made quite a few edits! The checks have passed, and I won't add more changes before your next review @gaborcsardi 😸

One more thing I've been wondering is whether the order of elements in comment should be consistent across authors, i.e. should e.g. ORCID always be the first element if there are several elements in comment. Really a detail though!

[gaborcsardi]

One more thing I've been wondering is whether the order of elements in comment should be consistent across authors, i.e. should e.g. ORCID always be the first element if there are several elements in comment.

I don't think it matters.

Thanks!

[gaborcsardi]

Looks really good, thanks much!