Dependencies

[srudkin12]

This is an excellent package and really helpful readme. Easy to follow and the results replicated quickly on my system.

When following the readme I was asked to install tidyverse and Rglpk but these are not mentioned in the readme as being needed.

[jonathandroth]

@mcaceresb can you look into this dependency issue? Thanks!

[mcaceresb]

@jonathandroth I added Rglpk as an explicit dependency in #23. A few packages have lifecycle as a dependency, which in turn has tidyverse as a dependency.

@srudkin12 tidyverse is only needed to the extent other dependencies need it. Rglpk is used directly and I added it as a dependency, but you shouldn't need to load it as a library separately after installing and loading HonestDiD.

[mcaceresb]

@jonathandroth @srudkin12 Actually I found another issue: I removed tidyverse from the library statements in the code in #23. I don't think any of the library statements are needed in general, but I hadn't noticed it tried to still load tidyverse.

[grantmcdermott]

Hey folks, I just had a look at this package and jumped on here to mention this same issue.

More generally, it's a big no-no to include any library statements at the top of internal R package scripts. These should all be removed. I can see that Mauricio started in #23, but the rest of them will have to go too. (Example.)

A lazy option would be to move these packages to Depends in the DESCRIPTION file, so that they auto-load along with this package. However, that too is not considered good practice and should only be done as holdover / last resort IMO.

Better would be to use explicit namespace assignment when you call a function, e.g. foreach::foreach... which I think you are already doing and so may not require any further work? I don't think you have a unit testing framework set up, but you should be able to run devtools::check() / R CMD check pretty quickly to at least test the documentation examples.

(You can also use importFrom statements as part of your docs, but I think the package::function approach is the easiest here.)

[mcaceresb]

@grantmcdermott Just flagging that I've done some work addressing these (and I have a local branch passing devtools::check() / R CMD check too). I'm waiting on my other PRs to be merged but all these things you mentioned are in the pipeline for being merged! (:

[grantmcdermott]

That's great, thanks @mcaceresb. I need to play around with the package a bit more, but might have some other ideas for how to speed up / simplify some internals.

[jonathandroth]

Grant, thanks for the feedback! And Mauricio, thanks for implementing!

Are you waiting on something from me / @Ashesh Rambachan @.***> re the PRs you mentioned?

On Thu, May 18, 2023 at 4:04 PM Grant McDermott @.***> wrote:

That's great, thanks @mcaceresb https://github.com/mcaceresb. I need to play around with the package a bit more, but might have some other ideas for how to speed up / simplify some internals.

— Reply to this email directly, view it on GitHub https://github.com/asheshrambachan/HonestDiD/issues/28#issuecomment-1553581222, or unsubscribe https://github.com/notifications/unsubscribe-auth/AE6EXFCQW6B2TZ3PWWX6SG3XGZ6DXANCNFSM6AAAAAAX4OSRO4 . You are receiving this because you were mentioned.Message ID: @.***>

[mcaceresb]

@jonathandroth No, they're ready to be merged.

[jonathandroth]

You have permissions to merge when ready, right? Or no?

On Thu, May 18, 2023 at 7:19 PM Mauricio Caceres Bravo < @.***> wrote:

@jonathandroth https://github.com/jonathandroth No, they're ready to be merged.

— Reply to this email directly, view it on GitHub https://github.com/asheshrambachan/HonestDiD/issues/28#issuecomment-1553773034, or unsubscribe https://github.com/notifications/unsubscribe-auth/AE6EXFABZVUW3BULQV2USZ3XG2U6TANCNFSM6AAAAAAX4OSRO4 . You are receiving this because you were mentioned.Message ID: @.***>

[mcaceresb]

@jonathandroth No, I don't have write access.

[asheshrambachan]

@mcaceresb Just added you as a collaborator (let me know if you got it). That you should give you write access to merge in pulls.