insightsengineering / teal.data

Data model for teal applications
https://insightsengineering.github.io/teal.data/
Other
8 stars 7 forks source link

CRAN comments #283

Closed gogonzo closed 7 months ago

gogonzo commented 7 months ago

Comments from CRAN reviewer.

The Description field is intended to be a (one paragraph) description of what the package does and why it may be useful. Please add more details about the package functionality and implemented methods in your Description text.

If there are references describing the methods in your package, please add these in the description field of your DESCRIPTION file in the form authors (year) authors (year) authors (year, ISBN:...) or if those are not available: with no space after 'doi:', 'arXiv:', 'https:' and angle brackets for auto-linking. (If you want to add a title as well please put it in quotes: "Title")

You have examples for unexported functions. Please either omit these examples or export these functions. Examples for unexported function default_cdisc_join_keys() in: col_labels.Rd

Please fix and resubmit.

chlebowa commented 7 months ago

The examples were accepted in teal.code. Is this because that was not the initial submission? If so, I take it the exmaples will be added back in the next release? Perhaps 0.4.1?

gogonzo commented 7 months ago

It might depend on a reviewer also.

perhaps 0.4.1

Yup, let's do this. But Today we can maybe move it to @details for a while - I'm waiting for opinion from CRAN reviewer.

chlebowa commented 7 months ago

Do you think they could waive the restriction if we explain that these are dev-oriented examples?

gogonzo commented 7 months ago

Do you think they could waive the restriction if we explain that these are dev-oriented examples?

I've tried explain this before with other package, I guess it was teal.code initial submission. I can't remember now and email has limited history also.

averissimo commented 7 months ago

Do you want to pre-emptively prepare that move to @details ?

I can do it quickly right now and target the release-candidate-v0.4.0 branch

gogonzo commented 7 months ago

Do you want to pre-emptively prepare that move to @details ?

Let's wait for answer from CRAN about this. We can address other comments first and wait for their opinion. I'll do respective changes over the weekend if answer comes too late 👍

averissimo commented 7 months ago

ok :crossed_fingers:

I'm curious about the rationale behind allowing documentation, but not examples. I can only think that they don't want to waste resources running these examples given that they have to test 10k+ packages regularly on multiple platforms

Still, it seems like they are promoting poor development practices by preventing better documentation

chlebowa commented 7 months ago

Probably those examples wouldn't run if we hadn't done the getFromNamespace trick. It makes me think it was an automated process: compare names of functions in examples against names of exported funcitons. But then why would it be allowed in teal.code? :thinking:

averissimo commented 7 months ago

We don't have any references describing the methods on teal.data so as far as I can tell there are only 2 actions required for resubmission

  1. Change/extend description in DESCRIPTION file
    • I'll propose a new one
  2. Sort out the @examples
    • Waiting for a reply from CRAN and act accordingly
m7pr commented 7 months ago

If they don't allow examples in the way we have them today, we can resubmit them in another CRAN release, as the next one does not have a human verification. We can do it even after a day or the first CRAN release.

gogonzo commented 7 months ago

If they don't allow examples in the way we have them today, we can resubmit them in another CRAN release, as the next one does not have a human verification. We can do it even after a day or the first CRAN release.

Good point. We can remove the examples IN ONE COMMIT and revert this commit easily later. We just need to keep the merged branch longer to preserver the commit (commits are squashed when merged to main). This is the fastest solution. Examples are just for us so we can address this later. So I suggest to remove examples now

averissimo commented 7 months ago

What exactly are the private examples here? I can't find any on teal.data

chlebowa commented 7 months ago

This?

Examples for unexported function default_cdisc_join_keys() in: col_labels.Rd

averissimo commented 7 months ago

Examples for unexported function default_cdisc_join_keys() in: col_labels.Rd

In col_labels.Rd all the calls/expressions on @examples are all to exported functions (col_labels<-, col_labels, col_relabel, get_labels)

default_cdisc_join_keys is exported (but not a function nor does it have @examples)

averissimo commented 7 months ago

And the only getFromNamespace hit is when we import lang2calls (R/zzz.R)

m7pr commented 7 months ago

let's submit without the change in examples then haha