Added Author Coercion #46
Conversation
R/authors-at-r.R
Outdated
| } | ||
| if (has_author & has_authors_at_r) { | ||
| # Delete Author as it has Authors@R | ||
| obj$del("Author") |
There was a problem hiding this comment.
You cannot modify obj, it will change (the representation of) the DESCRIPTION file.
There was a problem hiding this comment.
I don't understand completely. Can you not remove that field? I can simply add the Authors@R field, but then the description will both have Author and Authors@R.
I don't understand if you cannot modify the result within the function or if you do not want to use del in the function. I removed any deletion in the recent pushes. Is it OK if you do self$set_authors or is that modification undesirable as well?
There was a problem hiding this comment.
You cannot remove or add fields. This function is called from $get_authors(), which cannot modify the underlying data. It can return an answer based on Authors@R or Author, but cannot add or remove fields.
There was a problem hiding this comment.
Just think about it. If you modify it, then desc::desc_get_authors() rewrites the DESCRIPTION file. And we definitely don't want that.
|
I don't know TBH. I guess we can give it a try. |
|
But you need to fix this, so that it does not modify the object. |
|
Would it be problematic if this is a standalone function and not placed in
ensure_authors_at_r?
John
…On Fri, Jul 14, 2017 at 11:47 AM, Gábor Csárdi ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In R/authors-at-r.R
<#46 (comment)>:
> if (! ***@***.***")) {
stop("No ***@***.***' field!\n",
"You can create one with $add_author")
}
}
+coerce_authors_at_r <- function(obj) {
+ has_authors_at_r = ***@***.***")
+ has_author = obj$has_fields("Author")
+ if (! (has_authors_at_r | has_author) ) {
+ stop("No ***@***.***' or 'Author' field!\n",
+ "You can create one with $add_author")
+ }
+ if (has_author & has_authors_at_r) {
+ # Delete Author as it has ***@***.***
+ obj$del("Author")
Just think about it. If you modify it, then desc::desc_get_authors()
rewrites the DESCRIPTION file. And we definitely don't want that.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#46 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABBnrsgB50XkJnYKfEb8E2Lkx7j4IMVzks5sN414gaJpZM4OYZsJ>
.
|
Codecov Report
@@ Coverage Diff @@
## master #46 +/- ##
==========================================
+ Coverage 98.85% 98.87% +0.02%
==========================================
Files 18 18
Lines 1047 1071 +24
==========================================
+ Hits 1035 1059 +24
Misses 12 12
Continue to review full report at Codecov.
|
|
I thought the goal was to "fix" So what would this function do then? Convert |
|
Yeah - I'm being confusing.
The function would convert Author & Maintainer to Authors@R.
I guess I see that users would use coerce on their package if they got the
get_authors() error. Similarly do this before pkgdown::build_site()
Thanks
John
…On Fri, Jul 14, 2017 at 12:01 PM, Gábor Csárdi ***@***.***> wrote:
I thought the goal was to "fix" get_authors().
So what would this function do then? Convert Author & Maintainer to
***@***.***?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#46 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABBnrsuIHULRklsuc773VWruW3AVJyJyks5sN5DmgaJpZM4OYZsJ>
.
|
|
Yeah, I think some `$convert_to_authors_at_r()` function would make sense.
G.
On Fri, Jul 14, 2017 at 5:05 PM, John Muschelli <notifications@github.com>
wrote:
… Yeah - I'm being confusing.
The function would convert Author & Maintainer to ***@***.***
I guess I see that users would use coerce on their package if they got the
get_authors() error. Similarly do this before pkgdown::build_site()
Thanks
John
On Fri, Jul 14, 2017 at 12:01 PM, Gábor Csárdi ***@***.***>
wrote:
> I thought the goal was to "fix" get_authors().
>
> So what would this function do then? Convert Author & Maintainer to
> ***@***.***?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#46 (comment)>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/
ABBnrsuIHULRklsuc773VWruW3AVJyJyks5sN5DmgaJpZM4OYZsJ>
> .
>
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#46 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAoTQIYzVymCYZe_hXu3rbq1MQr3VlP8ks5sN5FrgaJpZM4OYZsJ>
.
|
|
This should have the coverage needed with the test. |
|
Sorry - it NOW has passed all checks for coverage and travis/appveyor. |
|
I think this should be ready for merging @gaborcsardi if you want - checks passed and coercion method implemented. |
…ed as per the PR. Merge remote-tracking branch 'upstream/master' # Conflicts: # DESCRIPTION # tests/testthat/D6
|
Hey @gaborcsardi - any thoughts on integrating this? I just re-integrated and resolved the conflicts for the authors coercion. |
|
TBH I am also not crazy about the name of the method, I'll think about that. |
|
Do you want to change the name? |
Merge remote-tracking branch 'upstream/master'
|
I have merge with upstream, I believe all changes have been addressed. |
|
I believe all issues are resolved @gaborcsardi |
|
It would be great to get this updated / merged. Having old style Authors fields in DESCRIPTION is a common issue for building pkgdown sites. |
Merge remote-tracking branch 'upstream/master'
|
I think this is ready to go - can you re-review? |
|
Thanks and I am sorry for the long wait! |
Added a parser to coerce Author to Authors@R in reference to issue #44. Let me know if this is desirable or wanted in the package. Otherwise, I can use this function in my own work.