PR for #17: Test package and review its documentation

[santiagohermo]

This is a review for #17, where we tested the package and implemented many enhancements to the code and the documentation. Thanks @mzwu @elianasena @ew487 for the help! See the thread in #17 for discussion on the changes. The latest updates can be found in https://github.com/JMSLab/eventstudyr/issues/17#issuecomment-1465071716.

The goal of the review is to:

• Double-check changes in code and documentation.
• Make sure the package works correctly after changes.
• If there are missing comments in the code, address them.

I added @mzwu as review and @nateschor and @santiagohermo as assignees. I expect @nateschor and @santiagohermo to also review the code though. Finally, I added @jmshapir as reviewer. Not sure if this is the optimal task assignment, so feel free to make changes!

[jmshapir]

I added @mzwu as review and @nateschor and @santiagohermo as assignees. I expect @nateschor and @santiagohermo to also review the code though. Finally, I added @jmshapir as reviewer. Not sure if this is the optimal task assignment, so feel free to make changes!

@santiagohermo I will take a pass over the changes here.

I removed @mzwu as reviewer but we can ask @mzwu to review specific changes made in response to @mzwu's comments if needed.

@SimonFreyaldenhoven @chansen776 @jorpppp after this pull is completed I plan to take the repository public, so if you'd like to review before then, please self-assign, thanks!

[jmshapir]

• When we reference the paper, we current do so as follows:

  [Freyaldenhoven et al. (2021)](https://www.nber.org/system/files/working_papers/w29170/w29170.pdf)

  This points to the pdf version. Should we instead point to the landing page of the paper, i.e., this one? Maybe this question is relevant for @jmshapir as well.

@santiagohermo sounds good! That way the URL will work even if users do not have access to NBER WPs.

Can you implement?

[nateschor]

@santiagohermo unless you've already started implementing the link change suggestions above, I can make the changes!

[santiagohermo]

@santiagohermo unless you've already started implementing the link change suggestions above, I can make the changes!

Thanks @nateschor! I was just coming back to this now. I'll let you implement the link updates then.

[santiagohermo]

In the end I went ahead and implement the updates to links @nateschor. See https://github.com/JMSLab/eventstudyr/pull/24/commits/d5857d5bed2e23f1a265e34379b35a3d216cdc4f. Thanks!

[santiagohermo]

Thanks @jmshapir @nateschor! It looks like the only remaining comments are here (aesthetic change to code) and here (reminder to remove issue folder). I'd be happy to wrap up once we complete the first one :)

[jmshapir]

Thanks @santiagohermo!

I implemented the remaining aesthetic change and approved the pull.

I suggest we give everyone roughly ~24 hours to raise any remaining objections.

If none arise, you can remove the issue subfolder and merge the pull, after which I will take the repository public and we can start next steps.

@rcalvo12 @nateschor @ew487 @SimonFreyaldenhoven @chansen776 @jorpppp

[santiagohermo]

Thanks @jmshapir! Your plan sounds good to me. If nothing comes up by tomorrow morning I'll proceed to wrap up.

[nateschor]

@santiagohermo the only things left on my end is seeing if there is a way we can make the vignette available to users as part of our GitHub release. I did some searching in https://github.com/JMSLab/eventstudyr/issues/17#issuecomment-1464335064 but haven't figured out how to get the vignette included as part of the README. Do you have any ideas?

We also have to fix the README example here

Otherwise, everything looks great to me and I'm ready to wrap up the PR and go live, thank you!

[santiagohermo]

Thanks @nateschor!

@santiagohermo the only things left on my end is seeing if there is a way we can make the vignette available to users as part of our GitHub release. I did some searching in #17 (comment) but haven't figured out how to get the vignette included as part of the README. Do you have any ideas?

Great point. I think having a pkgdown website would be great. It could be the first issue after we go public :)

[nateschor]

Per call:

@santiagohermo will wrap up the pull request. After we go live, we'll:

1. take a look at the Roadmap and see which tasks we can logically group together into issues (e.g, static model, CRAN submission prep)
2. iterate as needed on which tasks to prioritize and group together
3. create issues and assign people to those issues

[santiagohermo]

Summary here.

Looks like we are ready to go public @jmshapir @nateschor. When we do please let us know @jmshapir, so that we can make sure that installing the package with devtools works.

[jorpppp]

This looks great. I'll ask somebody to try to install using devtools.

For the public version, don't forget to choose a license (Probably MIT) and to create a release.

[nateschor]

Thanks @jorpppp!

I'll ask somebody to try to install using devtools.

Just noting that they won't be able to install the package using devtools until we make the repo public

[santiagohermo]

Thanks @jorpppp! That's actually a great point.

I created a draft for a release here @nateschor @jmshapir. Feel free to implement changes. We can publish the draft after going public.

[SimonFreyaldenhoven]

@santiagohermo @nateschor this is a bit random - but can you confirm that the Stata files in this subfolder are supposed to be included in the release/ that we still use them? For example, this file, which also seems to include pointers to a folder on @ew487's computer.

[santiagohermo]

Thanks for flagging that folder @SimonFreyaldenhoven! I hadn't looked into it before.

@nateschor can confirm, but my reaction is that those do files create input files for the tests that ensure that eventstudyr returns the same estimates as xtevent. See, for example, this test.

I agree that the link to @ew487's computer should probably not be part of the release. What do you think if I proceed as follows @SimonFreyaldenhoven @nateschor @jmshapir?

1. Create a new issue for the release.
2. Drop the reference to @ew487's computer and re-run those do files.
3. Have a quick PR.
4. Go public as part of that issue, and publish the release draft.
5. In that issue test the devtools installation after going public, and discuss other things that may come up.

[jmshapir]

@santiagohermo @nateschor @SimonFreyaldenhoven @jorpppp thanks!

@santiagohermo the plan you outline in https://github.com/JMSLab/eventstudyr/pull/24#issuecomment-1474381546 sounds great to me, thanks, and I'm happy to review/edit the release as part of the follow-up issue.

[SimonFreyaldenhoven]

@santiagohermo thanks! Got it - that both makes sense to me and sounds like a plan!