search

ropensci / software-review

rOpenSci Software Peer Review.
286 stars 104 forks source link

Submission of naijR 0.6.1 for review under the rOpenSci Champions Program #600

Closed BroVic closed 7 months ago

BroVic commented 10 months ago

Date accepted: 2023-12-02

Submitting Author Name: Victor Ordu Submitting Author Github Handle: !--author1-->@BroVic<!--end-author1--

Other Package Authors Github handles: (comma separated, delete if none) Repository: https://github.com/BroVic/naijR

Version submitted: 0.6.1

Submission type: Standard

Editor: !--editor-->@emilyriederer<!--end-editor--

Reviewers: @mcsiple, @cagrigokcek

Archive: TBD Version accepted: TBD Language: en

Package: naijR
Type: Package
Title: Operations to Ease Data Analyses Specific to Nigeria
Version: 0.6.1
Depends: R (>= 3.6)
Imports:
    RColorBrewer (>= 1.1.2),
    cli (>= 3.6.0),
    lifecycle (>= 0.2.0),
    grDevices,
    graphics,
    mapdata (>= 2.3.0),
    maps (>= 3.3.0),
    rlang (>= 0.4.0),
    sf (>= 1.0.13),
    stats,
    stringi (>= 1.7.6),
    utils
Suggests: 
    ISOcodes,
    covr,
    here,
    knitr,
    purrr,
    readxl,
    rmarkdown,
    testthat (>= 3.0.0),
    usethis
Authors@R: person("Victor", "Ordu", 
              email = "victorordu@outlook.com",
              role = c("aut", "cre"),
              comment = c(ORCID = "0000-0003-3716-0668"))
Description: A set of convenience functions as well as geographical/political
  data about Nigeria, aimed at simplifying work with data and information that
  are specific to the country.
License: GPL-3
LazyData: TRUE
Encoding: UTF-8
RoxygenNote: 7.2.3
VignetteBuilder: knitr
RdMacros: lifecycle
URL: https://brovic.github.io/naijR/
BugReports: https://github.com/BroVic/naijR/issues
Config/testthat/edition: 3

Scope

Technical checks

Confirm each of the following by checking the box.

This package:

Publication options

MEE Options - [ ] The package is novel and will be of interest to the broad readership of the journal. - [ ] The manuscript describing the package is no longer than 3000 words. - [ ] You intend to archive the code for the package in a long-term repository which meets the requirements of the journal (see [MEE's Policy on Publishing Code](http://besjournals.onlinelibrary.wiley.com/hub/journal/10.1111/(ISSN)2041-210X/journal-resources/policy-on-publishing-code.html)) - (*Scope: Do consider MEE's [Aims and Scope](http://besjournals.onlinelibrary.wiley.com/hub/journal/10.1111/(ISSN)2041-210X/aims-and-scope/read-full-aims-and-scope.html) for your manuscript. We make no guarantee that your manuscript will be within MEE scope.*) - (*Although not required, we strongly recommend having a full manuscript prepared when you submit here.*) - (*Please do not submit your package separately to Methods in Ecology and Evolution*)

Code of conduct

emilyriederer commented 7 months ago

Hi @BroVic ! I hope you are doing much better. I just wanted to check in and see when you thought you might be ready to circle back to this? Or do you think we should put it on pause?

BroVic commented 7 months ago

Hello @emilyriederer. I'm sorry for the late response. I must have missed the notification. No, I will make my submission in the next few days. And thanks for enquiring into my health. I'm fully recovered now.

Edit: I got the notification to your message just 12 hours ago! So, there's something wrong at GitHub's end.

BroVic commented 7 months ago

Dear @cagrigokcek, thanks again for your review. I would like to make a few comments and if you think I am inadequately informed, feel free to let me know:

  1. Although this package has a function that draws maps, this is not its main purpose. The central idea was to provide data that are specific to Nigeria and to help users work better with those data. The map_ng() function returns an sf object, which makes it easier to use with other real geospatial packages.
  2. I took a look at how best one could link the package to {BrailleR} as suggested. My preferred approach would have been to run the output of map_ng() through ggplot::geom_sf() and from there attempt to get textual data from the map. I actually thought that ggplot2 provides methods for working with {BrailleR}. I can now see that methods like BralleR::Describe are defined by that package's author(s). So, I believe the onus seems to lie with them to create methods that can generically provide accessible output from objects created by widely used and popular packages like {ggplot2}

So, in light of the foregoing, I might have to decline this feature for {naijR}, the reason being that it is out of scope (for now). But knowing how important this issue of accessibility is, I will bear it in mind in future development of this package, and if a decision is made to expand the scope, then one might consider taking on {BrailleR} as a dependency.

Cheers.

BroVic commented 7 months ago

Hello @mcsiple, thank you very much for your review. It enabled me to effect several improvements and opened my eyes to other issues you didn't mention directly.

I have 2 comments:

  1. I am aware that sapply is not type-safe, but in this case I would plead that we let it be as it is. The places where it is used are not accessible to the end user - those are utility files for generating some of the package's internal data. I had to use the function because I can return a named object and I made sure it returns a list via the simplify = FALSE argument. The other option would have been to use the {purrr} package, but I don't believe this justified adding the dependency. If I tried, using other functions, everything just broke.
  2. Unfortunately, there is no canonical source for the correct versions of the administrative region names. Even the Nigerian Constitution, where the Local Government Areas are listed, overt mistakes as well as contested names! Right now, they are working on a law that addresses these naming discrepancies, and this is one of the things that made me develop this package in the first place. So, it's still evolving and when we have something more concrete, I will definitely implement your suggestion.

Once again, thanks!

emilyriederer commented 7 months ago

Hi @cagrigokcek and @mcsiple - apologies for the delayed timeline on this review cycle! If you have the bandwidth to pull your mind back to this project, I'd really appreciate your thoughts on @BroVic 's comments above.

mcsiple commented 7 months ago

Hi @BroVic and @emilyriederer , sorry for the delay. I was out of the office for a longer period than usual and am just getting back to my emails and GitHub. @BroVic , I am fine with those two comments. For (1) I agree that it's not worth adding a dependency (and I think purrr is sometimes slower than sapply()) so that is okay. And for (2) I am impressed at the ways you have found to include this 'naming flexibility' in your package. That is an exciting software challenge indeed 🙂

Anyway, I am satisfied with the changes and your comments.

emilyriederer commented 7 months ago

Thanks so much @mcsiple ! I hope you had a nice break from the office.

If both you and @cagrigokcek could please use the reviewer approval form once you are satisfied, that will be the final step!

cagrigokcek commented 7 months ago 
            Hi Emily, May I request a few days extension because I struggle with a severe tooth pain which decreases my concentration significantly? I promise to complete the form before the weekend.

Best.

Cagri

From: Emily Riederer @.> Sent: Sunday, November 26, 2023 5:47 PM To: ropensci/software-review @.> Cc: A. Cagri gokcek @.>; Mention @.> Subject: Re: [ropensci/software-review] Submission of naijR 0.6.1 for review under the rOpenSci Champions Program (Issue #600)

Thanks so much @mcsiple https://github.com/mcsiple ! I hope you had a nice break from the office.

If both you and @cagrigokcek https://github.com/cagrigokcek could please use the reviewer approval form https://devguide.ropensci.org/approval2template.html once you are satisfied, that will be the final step!

— Reply to this email directly, view it on GitHub https://github.com/ropensci/software-review/issues/600#issuecomment-1827005264 , or unsubscribe https://github.com/notifications/unsubscribe-auth/A3LYXAASB7EHR6XI4AN3UXTYGPWIFAVCNFSM6AAAAAA3OYM35WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRXGAYDKMRWGQ . You are receiving this because you were mentioned.Message ID: @.***>

emilyriederer commented 7 months ago

Of course, @cagrigokcek ! Sorry to hear that and please take the time that you need.

BroVic commented 7 months ago

@cagrigokcek Sorry about your tooth. Do take care and get well soon.

cagrigokcek commented 7 months ago

Hi viktor,

I am so sorry to see your message very late. I totally understand your situation about BrailleR package. As you said, it can be a further goal for you to develop packages which can be integrated with available packages related to accessibility in R later. I find your explanation very legitimate. Please don’t hesitate to reach me out when you need a tester in the future about features related to accessibility in R.

Best.

Cagri

From: Victor Ordu @.> Sent: Thursday, November 16, 2023 8:46 AM To: ropensci/software-review @.> Cc: A. Cagri gokcek @.>; Mention @.> Subject: Re: [ropensci/software-review] Submission of naijR 0.6.1 for review under the rOpenSci Champions Program (Issue #600)

Dear @cagrigokcek https://github.com/cagrigokcek , thanks again for your review. I would like to make a few comments and if you think I am inadequately informed, feel free to let me know:

  1. Although this package has a function that draws maps, this is not its main purpose. The central idea was to provide data that are specific to Nigeria and to help users work better with those data. The map_ng() function returns an sf object, which makes it easier to use with other real geospatial packages.
  2. I took a look at how best one could link the package to {BrailleR} as suggested. My preferred approach would have been to run the output of map_ng() through ggplot::geom_sf() and from there attempt to get textual data from the map. I actually thought that ggplot2 provides methods for working with {BrailleR}. I can now see that methods like BralleR::Describe are defined by that package's author(s). So, I believe the onus seems to lie with them to create methods that can generically provide accessible output from objects created by widely used and popular packages like {ggplot2}

So, in light of the foregoing, I might have to decline this feature for {naijR}, the reason being that it is out of scope (for now). But knowing how important this issue of accessibility is, I will bear it in mind in future development of this package, and if a decision is made to expand the scope, then one might consider taking on {BrailleR} as a dependency.

Cheers.

— Reply to this email directly, view it on GitHub https://github.com/ropensci/software-review/issues/600#issuecomment-1814830096 , or unsubscribe https://github.com/notifications/unsubscribe-auth/A3LYXAGNFIHXIXHSAO5Q2TTYEY7LZAVCNFSM6AAAAAA3OYM35WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJUHAZTAMBZGY . You are receiving this because you were mentioned.Message ID: @.***>

cagrigokcek commented 7 months ago

Hi Emily and Viktor,

I copy my review approval form below. Please let me know if there is any missing part in the form.

I congratulate Viktor again.

Best.

Cagri

Estimated hours spent reviewing: 5h30m

From: Emily Riederer @.> Sent: Sunday, November 26, 2023 5:47 PM To: ropensci/software-review @.> Cc: A. Cagri gokcek @.>; Mention @.> Subject: Re: [ropensci/software-review] Submission of naijR 0.6.1 for review under the rOpenSci Champions Program (Issue #600)

Thanks so much @mcsiple https://github.com/mcsiple ! I hope you had a nice break from the office.

If both you and @cagrigokcek https://github.com/cagrigokcek could please use the reviewer approval form https://devguide.ropensci.org/approval2template.html once you are satisfied, that will be the final step!

— Reply to this email directly, view it on GitHub https://github.com/ropensci/software-review/issues/600#issuecomment-1827005264 , or unsubscribe https://github.com/notifications/unsubscribe-auth/A3LYXAASB7EHR6XI4AN3UXTYGPWIFAVCNFSM6AAAAAA3OYM35WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRXGAYDKMRWGQ . You are receiving this because you were mentioned.Message ID: @.***>

emilyriederer commented 7 months ago

Congratulations @BroVic ! It looks like both reviewers have approved your package. (@mcsiple - would appreciate if you could fill out the last template just to close the loop, but I appreciate that we have your answer!) I will trigger the Approval bot now. Please review the messages it sends carefully with instructions for next steps to onboard this package to the rOpenSci family

emilyriederer commented 7 months ago

@ropensci-review-bot approve najiR

ropensci-review-bot commented 7 months ago

Approved! Thanks @BroVic for submitting and @mcsiple, @cagrigokcek for your reviews! :grin:

To-dos:

Should you want to acknowledge your reviewers in your package DESCRIPTION, you can do so by making them "rev"-type contributors in the Authors@R field (with their consent).

Welcome aboard! We'd love to host a post about your package - either a short introduction to it with an example for a technical audience or a longer post with some narrative about its development or something you learned, and an example of its use for a broader readership. If you are interested, consult the blog guide, and tag @ropensci/blog-editors in your reply. They will get in touch about timing and can answer any questions.

We maintain an online book with our best practice and tips, this chapter starts the 3d section that's about guidance for after onboarding (with advice on releases, package marketing, GitHub grooming); the guide also feature CRAN gotchas. Please tell us what could be improved.

Last but not least, you can volunteer as a reviewer via filling a short form.

mcsiple commented 7 months ago

Oh and here's the form in case you need it:

Reviewer Response

Final approval (post-review)

Estimated hours spent reviewing: 5.5

BroVic commented 7 months ago

@ropensci-review-bot finalize transfer of BroVic/naijR

ropensci-review-bot commented 7 months ago

Can't find repository ropensci/BroVic/naijR, have you forgotten to transfer it first?

BroVic commented 7 months ago

@ropensci-review-bot finalize transfer of naijR

ropensci-review-bot commented 7 months ago

Transfer completed. The naijR team is now owner of the repository and the author has been invited to the team

BroVic commented 7 months ago

Thank you @emilyriederer!