R opensci editor comments

[RayStick]

Closes #

References #

Responding to rOpenSci editor comments: ropensci/software-review#674 (comment)

Proposed Changes

PLEASE NOTE: Improvements to DESCRIPTION and README, and considerations for changing package name, will be addressed elsewhere.

• I'd recommend removing the "Maintainer" field from DESCRIPTION as Authors@R already covers it. 👉 03fc91a
• I'd recommend running desc::desc_normalize() to normalize DESCRIPTION. For instance the dependencies are not alphabetically ordered yet. 👉 9d030b4
• I like how welcoming the contributing guide is!

Thank you ❤️. I was taking a look at it again and made some minor updates 0dc469f

• For opening an URL in the browser, you can recommend (or even add in a cli .run message) browseURL(). 👉 957e019
• Instead of suggesting users start from an empty folder, could the functionality be integrated with for instance usethis::create_project()?

24bb25d I realised that there was no specific reason why a user has to create an empty folder - it is just neater that way. As the function is not dependent on an empty folder, I have just deleted this section in the README. Do you still think it is best practice to include usethis::create_project() ?

• Why list internal functions in the pkgdown reference index?

No good reason, removed! fdfac58

• In tests, individual test files do not have to load libraries. https://github.com/aim-rsf/browseMetadata/blob/main/tests/testthat/test-browse_metadata.R

I have now removed loading of libraries in each test file, and the testhat.R file contains:

library(testthat)
library(browseMetadata)
library(mockery)
library(withr)

My understanding is that I still need to load mockery amd withr, because they are not auto loaded by the package (just in 'Suggets') - let me know if I interpretted incorrectly here

• Instead of tempdir() it is safer to use withr::local_tempfile() that automatically cleans up after itself. Context: https://testthat.r-lib.org/articles/test-fixtures.html (so you could get rid of unlink(c(table_file, bar_file_html, bar_file_csv)) -- that's personal preference though. 👉 9906266 and 639a2b9
• expect_true(nrow(bar_csv) == 26)) could be expect_equal(nrow(bar_csv), 26)). I like reading this once in a while because I easily forget about shorter expectations (and there are new ones). 😅 Actually expect_equal() is used in another file. Actually some linters might help you catch such cases

Changed to expect_equal ea6b5df and ran some linters which suggested different changes 04fc2dd

• For mocking you might like the new support for mocking in testthat https://testthat.r-lib.org/reference/local_mocked_bindings.html (see also https://blog.r-hub.io/2024/03/21/mocking-new-take/)

I have now used local_mocked_bindings for many of the tests. It saved lines, so that was good. I could not work out how to use local_mocked_bindings for 3 tests files (test-user_prompt.R, test-user_categorisation.R, test-user_prompt_list.R) so I kept the old functions which use mockery.

• Regarding https://github.com/aim-rsf/browseMetadata/blob/57b7191b967870160f06dcbec59fd58f36cbbb23/R/map_metadata.R#L60 why not simply make getwd() the default for the output_dir argument? This way it's clearer to users what the default is.

This is great, thanks. I changed it for 3 functions: d0a3725

Checklist for the author of this PR:

• [if package files were edited] I have run these checks locally:
  • devtools::document() to generates the .Rd files from any updated roxygen comments.
  • codemetar::write_codemeta() to ensures the metadata file is up to date.
  • styler::style_pkg() to ensure consistent code styling that match the guidelines.
  • devtools::check() for a comprehensive package check. I have resolved any warnings or errors, or written them here in the PR, for discussion.
• The code base and the documentation files match (they both reflect any recent changes).
• The title of this PR is clear and self-explantory.
• I have added any appropriate labels to this PR.
• This PR is now ready for review (and I have removed the draft PR status).

[codecov]

Codecov Report

Attention: Patch coverage is 95.00000% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
R/map_metadata.R	50.00%	1 Missing ⚠️

Files with missing lines	Coverage Δ
R/browse_metadata.R	94.84% <100.00%> (+0.78%)	⬆️
R/data_manipulation.R	100.00% <100.00%> (ø)
R/map_metadata_compare.R	100.00% <100.00%> (+1.23%)	⬆️
R/map_metadata_convert.R	100.00% <100.00%> (ø)
R/user_interactions.R	96.74% <100.00%> (ø)
R/map_metadata.R	70.31% <50.00%> (+0.20%)	⬆️

[RayStick]

Unsure why test-coverage is failing, seemed to start failing on this commit 04fc2dd ?

Strangely all the unit tests pass for me locally

@maelle I am a bit stuck debugging this error - feel free to review the PR only after its solved if you prefer

[RayStick]

@maelle all the tests pass now 🥳 (I found the bug)
Feel free to review when you have time to do so

[maelle]

Thank you, the changes look good!

Regarding the user prompt, should it use utils::menu() for the y/n answers? (this might also be useful for user_prompt_list()?) I was looking at https://github.com/r-lib/usethis/blob/d3b4a3c216d1e83b9b67f42e1eff352a429ede31/R/utils-ui.R as I have no experience with such functions myself but remembered seeing them as an user of usethis. (for free text I see https://github.com/r-lib/gitcreds/blob/cf006f0ae28d62ad2f9d24b03bbd0e99f7770764/R/git-auth.R#L111 uses the same thing as you do) I started looking into this as you mentioned tests. I am not sure I would have tested those but that doesn't mean testing them is a bad idea. 😸

[RayStick]

Regarding the user prompt, should it use utils::menu() for the y/n answers? (this might also be useful for user_prompt_list()?) I was looking at https://github.com/r-lib/usethis/blob/d3b4a3c216d1e83b9b67f42e1eff352a429ede31/R/utils-ui.R as I have no experience with such functions myself but remembered seeing them as an user of usethis. (for free text I see https://github.com/r-lib/gitcreds/blob/cf006f0ae28d62ad2f9d24b03bbd0e99f7770764/R/git-auth.R#L111 uses the same thing as you do) I started looking into this as you mentioned tests. I am not sure I would have tested those but that doesn't mean testing them is a bad idea. 😸

Ohh, interesting. I think it would take me another PR and a bit of time to understand how these utils:meun() functions can help, as I have lots of user interaction all throughout the package. What do you recommend - shall I put it as an Issue in the repo, or address before/during the review process? Thanks!

[maelle]

I think utils::menu() might simplify your code, but that's for you to decide and absolutely fine to prefer waiting.

[RayStick]

I think utils::menu() might simplify your code, but that's for you to decide and absolutely fine to prefer waiting.

Yeah, I think so too! If it's okay to delay to January for sending for review, this week I can

1. Work on using the utils:menu() to simplify the code base
2. Work on updating the README for more package context (this doesn't sound like a big job but unfortunately I think my code might have to change as I noticed the input file structure on the latest metadata catalogue has changed)

[maelle]

Can you please ask this question again in the software-review thread? Spoiler, I'll answer yes. 😁 Note that I'll be on vacation from December 28th to January 6th.

[RayStick]

@maelle I am going to merge this PR as I think I have responded to all your feedback (either implementing the change or making an issues) - any reason not to at this stage? Thanks for the feedback so far!

[maelle]

Sounds good and my pleasure!