Address feedback from CRAN

[nateschor]

Based on https://github.com/JMSLab/eventstudyr/pull/32#pullrequestreview-1377856245, we'll make any changes CRAN asks for in this issue and close the issue when we are accepted to CRAN

[nateschor]

@ew487 it looks like CRAN would like the maintainer to be the person who does the submitting. Can you do the honors?

Please make sure you've pulled the most recent changes into main, and then run devtools::submit_cran(). Let me know if you have any questions!

Thank you!

@jmshapir FYI

[ew487]

@nateschor Just to make sure, we don't want to use devtools::release() and do the extra checks?

[nateschor]

@ew487 nice find! More checks sounds great! How's this for a plan:

1. Try devtools::release(check = TRUE)
2. Let us know if it's flagging things that you think we should change and we'll make the changes here
3. Submit to CRAN afterwards
   • either with release(), or with submit_cran() if release() is giving you a hard time

Thank you!

[nateschor]

Per call: @ew487 will keep working through the suggestions from devtools::release(check = TRUE), prioritizing checks that return a failure and skipping check suggestions that seem unnecessary.

[jmshapir]

@nateschor @ew487 thanks! Is it possible to post a screenshot or other copy of the checks we're talking about?

[ew487]

@jmshapir @nateschor I tried to print out a list of checks by running usethis::use_release_issue(), but instead it opened a new issue in github. So that it why there is an issue #34 .

[ew487]

@jmshapir Here is a screenshot of some of the checks:

[jmshapir]

@jmshapir @nateschor I tried to print out a list of checks by running usethis::use_release_issue(), but instead it opened a new issue in github. So that it why there is an issue #34 .

@ew487 thanks! I removed that issue so no worries. :-)

[ew487]

[ew487]

Link to CRAN policies: https://cran.r-project.org/web/packages/policies.html

[jmshapir]

@ew487 thanks!

I will read those policies and follow up.

@nateschor have we already run R CMD check --as-cran? If not could you give that a try and let us know if there are any fatal errors that would prevent submission?

[nateschor]

@nateschor have we already run R CMD check --as-cran? If not could you give that a try and let us know if there are any fatal errors that would prevent submission?

@jmshapir I have not explicitly run R CMD check --as-cran, but based on this (screenshotted below) and the devtools::check() documentation I think we have been running R CMD check --as-cran via devtools::check().

Screenshot


I tried running from the command line below but the error about not having author or maintainer indicates to me that I'm not running the command correctly because we clearly do have author and maintainer in the DESCRIPTION file

Screenshot


All this to say that I think we've satisfied this checking requirement from CRAN! Please let me know though if you disagree

[jmshapir]

@nateschor thanks for confirming!

@ew487 based on https://github.com/JMSLab/eventstudyr/issues/33#issuecomment-1508791957 and my reading of the policies I think you can check these boxes and upload!

[ew487]

@jmshapir @nateschor Great! I just uploaded it.

[nateschor]

@jmshapir @nateschor Great! I just uploaded it.

We're in the queue!

[nateschor]

@ew487 did you hear anything from CRAN?

I see our package in the archive folder here

Archive screenshot


According to the CRAN incoming dashboard, archive means "your package has been rejected from CRAN because it did not pass checks cleanly and the problems were not likely to be false positives."

CRAN status definitions


fyi @jmshapir

[ew487]

@nateschor Yes, I did! Here are the things we should fix before resubmitting.

[jmshapir]

@ew487 thanks!

I opened branch 33-address-feedback-from-cran for work here.

@nateschor two things:

1. Would you have bandwidth to switch from citEntry() to bibentry()?
2. Would you be able to switch the AER citation and NBER citations to DOI format?

[SimonFreyaldenhoven]

@jmshapir I understand the above to mean we should EITHER include a DOI or an arxiv ID. The NBER WP has a DOI, so I think if we add that we should be good?

[jmshapir]

@SimonFreyaldenhoven ah good point, thanks! I hadn't realized that the NBER assigns DOI links.

I revised my comment in https://github.com/JMSLab/eventstudyr/issues/33#issuecomment-1511496543.

[nateschor]

@jmshapir I can address https://github.com/JMSLab/eventstudyr/issues/33#issuecomment-1511496543 later today!

[nateschor]

@jmshapir I made the changes in https://github.com/JMSLab/eventstudyr/commit/68e179907c8255bb163c7b9639dfb4bc1785a3b2

• updated cran-comments.md with our resubmission information based on the suggestions here
• assumed that they wanted me to add the DOI information here, but please let me know if you think there is a different place I should be adding the citation information or if I've added it incorrectly

@santiagohermo can you confirm that I correctly switched to bibentry()? Is it as straightforward as swapping out the function without running any code or recompiling any .md files?

Thanks!

[santiagohermo]

Thanks @nateschor! It seems to me that just changing to bibentry as you did in https://github.com/JMSLab/eventstudyr/commit/68e179907c8255bb163c7b9639dfb4bc1785a3b2 is what we are being asked to do. We created the citation with usethis, interesting to see that it seems outdated.

I tested the new citation and it looks like this:

It looks like we need to add the year. Can you do it?

[nateschor]

@santiagohermo added year in https://github.com/JMSLab/eventstudyr/commit/ce901f2a0cfae960519161163924fcf296dbaa35!

Does it look okay to you now?

[santiagohermo]

Thanks @nateschor! Yes, it looks good to me. Hopefully it also looks good to CRAN :)

[jmshapir]

@ew487 can you resubmit and let us know any feedback? Thanks!

[nateschor]

@jmshapir I'm guessing we want to leave this issue open until after we're accepted to CRAN?

Given that we're:

• keeping this issue open
• adding a few, non-code commits

Do we still follow the standard PR workflow? Or should we not delete this issue branch when we squash merge?

Furthermore, do we need to increment the eventstudyr version number before submitting? Here suggests we do ("Increase the patch version of your package. Yes, this means that there might be gaps in your released version numbers. This is not a big deal."), because I can see it being confusing to CRAN that we're saying we've made changes to the package, but we're resubmitting with the same version number

[nateschor]

@jmshapir before @ew487 resubmits can we discuss here if we need to increment the package version number?

[jmshapir]

@nateschor thanks!

@jmshapir I'm guessing we want to leave this issue open until after we're accepted to CRAN?

Yep, or at least for the time being.

Given that we're:

• keeping this issue open
• adding a few, non-code commits

Do we still follow the standard PR workflow? Or should we not delete this issue branch when we squash merge?

I think we should not open a pull at the moment, and instead keep the issue and branch open pending a reply from CRAN.

Furthermore, do we need to increment the eventstudyr version number before submitting? Here suggests we do ("Increase the patch version of your package. Yes, this means that there might be gaps in your released version numbers. This is not a big deal."), because I can see it being confusing to CRAN that we're saying we've made changes to the package, but we're resubmitting with the same version number

I would not favor making a new github release at this time, but I would be fine incrementing the minor version in the package itself, on the #33 issue branch, if we think that is what CRAN will expect.

[nateschor]

@jmshapir thanks!

I would not favor making a new github release at this time, but I would be fine incrementing the minor version in the package itself, on the #33 issue branch, if we think that is what CRAN will expect.

What's your thinking for incrementing the minor version rather than the patch? Based on the semantic versioning document, the changes we made in this issue branch:

• don't add functionality
• are backwards compatible
• are bug fixes

which (to me) points towards a patch increase rather than a minor increase

[jmshapir]

@nateschor thanks! Incrementing the patch sounds great.

[nateschor]

I won't have a chance to work on this until after 2pm today, I'm not sure if it's as simple as changing the number here or if we need to change it with a function or in other places too. Feel free to implement before then if you're able to @santiagohermo, otherwise I can do it later!

[santiagohermo]

Version number increased in https://github.com/JMSLab/eventstudyr/commit/2eb500544326ae05f71bbff7d7470a78e3a9e41d @nateschor

[nateschor]

Thanks @santiagohermo!

@ew487 at least on my end you have the green light to resubmit with the version of the package we have in #33! @jmshapir are you also okay with @ew487 submitting?

[jmshapir]

@jmshapir are you also okay with @ew487 submitting?

Yes thanks all!

[ew487]

@jmshapir @nateschor @santiagohermo Thanks! Just resubmitted.

[jmshapir]

[nateschor]

@SimonFreyaldenhoven @jmshapir @rcalvo12 @santiagohermo @ew487 @veli-m-andirin we're on CRAN!!

I just ran install.packages("eventstudyr") and it worked! You can also see we're on the list of CRAN packages

[santiagohermo]

Cool @nateschor! I could install with install.packages as well.

In https://github.com/JMSLab/eventstudyr/commit/cf2365a0f5e05ac8407fd1f8ca888a48b6f3ea4e I updated the README to add the option to install from CRAN.

[nateschor]

In https://github.com/JMSLab/eventstudyr/commit/cf2365a0f5e05ac8407fd1f8ca888a48b6f3ea4e I updated the README to add the option to install from CRAN.

Thanks @santiagohermo!

Now that we've had our fun celebrating, back to work! Proposed next steps:

1. Close this issue and open a PR
2. After we've squash-merged, update the GitHub release to includes our acceptance to CRAN and the changes we made in #28 and here. I'm thinking the version number should either be 1.1.0 or 2.0.0 since we did introduce some non-backwards compatible changes by making PrepareLeads and PrepareLags internal.
3. @santiagohermo works on #31. I'll propose some things to tackle from the "Would Like" section here and from the Wiki, please jump in with suggestions @jmshapir @rcalvo12 @ew487

Thank you!

[SimonFreyaldenhoven]

@nateschor @santiagohermo @jmshapir Thanks everyone!

I took a quick look at the files that are up on CRAN now, in particular the Reference Manual, and noticed a couple of things:

1) A few hyperlinks appear to be broken, e.g.: 2) Is eta really the proxy variable? 3) Why do we need the four different datasets? For example, it looks as if df_EventStudyFHS_example and df_EventStudyOLS_example are identical, except that the FHS version has one extra variable (eta). Maybe we can consolidate into one df? Simialrly, is there a reason we need the other two dataframes? Right now we introduce a lot of datasets which look pretty similar before finally, on page 5, introducing the "actual package"

[jmshapir]

@nateschor @santiagohermo @SimonFreyaldenhoven thanks!

On next steps:

1. Close this issue and open a PR

Agreed.

2. After we've squash-merged, update the GitHub release to includes our acceptance to CRAN and the changes we made in Release to CRAN #28 and here. I'm thinking the version number should either be 1.1.0 or 2.0.0 since we did introduce some non-backwards compatible changes by making PrepareLeads and PrepareLags internal.

I'd probably keep the github version as of this release at 1.0.2 to align with the CRAN version.

3. @santiagohermo works on Revise functions PrepareLeads, PrepareLags, and GetFirstDifferences #31.

Agreed.

I'll propose some things to tackle from the "Would Like" section here and from the Wiki, please jump in with suggestions @jmshapir @rcalvo12 @ew487

@nateschor maybe you can start by opening an issue to tackle the points in https://github.com/JMSLab/eventstudyr/issues/33#issuecomment-1516352909?

More generally, now that the package is live I'd be in favor of breaking work into small chunks when possible, just so that we can wrap up issues somewhat quickly and allow users to access a current version from main.

Of course we shouldn't push every new github release to CRAN, but we can push every few months or so to keep CRAN somewhat in line with github.

[nateschor]

@jmshapir thanks! I agree with everything in https://github.com/JMSLab/eventstudyr/issues/33#issuecomment-1516500861 and I'll have some time to work on these steps this afternoon

[jorpppp]

Thanks everyone for the great work! Publishing on CRAN definitely requires a lot of patience. Congrats!

@Constantino-Carreto-Romero Can you please request installation of eventstudyr on our server?

Should we publicize eventstudyr on Twitter now @jmshapir @SimonFreyaldenhoven?

[nateschor]

Thread continues in https://github.com/JMSLab/eventstudyr/pull/35#issue-1677221306

[nateschor]

Summary

In this issue we submitted to CRAN, were rejected, and then accepted.

Helpful links for monitoring our CRAN status were the CRAN incoming dashboard and the CRAN server.

Stable link to the issue branch here

Merged into main in https://github.com/JMSLab/eventstudyr/commit/e430cf59ee0c2ac6adfe190c11dc6f432972d95a