Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clearer instructions & input checking (R1-3 and R2-4) #197

Closed
Tracked by #184
RayStick opened this issue Feb 14, 2025 · 0 comments · Fixed by #198
Closed
Tracked by #184

Clearer instructions & input checking (R1-3 and R2-4) #197

RayStick opened this issue Feb 14, 2025 · 0 comments · Fixed by #198
Assignees

Comments

@RayStick
Copy link
Contributor

RayStick commented Feb 14, 2025

Reviewer 2 - Point 4 - Expectations of functionality

[c] Demo mode is said to process only the first 20 variables from the selected table and it's not clear what the 20 variables refer to particularly when directed to selecting data elements. Are data elements the same as variables in this example? I got a bit confused with this so couldn't complete the instructions.

[d] The parameters used in the metadata_map() function example don't appear to work. The code is metadata_map(csv_file = new_csv_file, domain_file = demo_domains_file) but there isn't a parameter called csv_file - should this be metadata_file? I first of all tried to use the 14_Mortality (Death registration)_Metadata.csv data from the website but got the error because the use of structural metadata is only referred to in the making of metadata. I found this link via the arguments help for metadata_file which is excellent documentation, however, will be missed I think so would be really great to be clear in the README file as well. An enhancement may be to have a message coded if a file is missing structural in the file name that the error is because it's the wrong file?

RELATED - [Reviewer 1- point 3] Input checking

I suggest adding explicit input validation to your exported functions. This would enhance the user experience by providing clear and informative error messages or warnings.

  • For map_compare(), consider verifying that session_dir exists and that the CSV files specified in the user input are present.
  • For map_convert(), you might want to check that both output_csv and output_dir exist.
  • For metadata_map() and the other functions, validating file existence, expected dimensions, and column names (if any) could further improve robustness.

[e] get("look_up") should perhaps come earlier in the vignette because it's not possible to use this function when in the process of selecting variables from using metadata_map() that appears just before.

README and vignette now been arranged (previous PR)

[f] For the prompt Enter one or more numbers separated by spaces and then ENTER, or 0 to cancel I tried just using the Enter/Return key to finish which gave an output. Pressing Enter/Return key though isn't a listed option and I just guessed it would work so possibly better to be explicit.

Fair point! This wording is from utils::select.list package so I cannot change it easily

[g] I recommend linking to the function reference for map_compare() in tutorial.

[h] I recommend being explicit in the argument help for metadata_file as ?metadata won't return any files if the package hasn't been loaded: ?mapmetadata::metadata. Although this is functionality in RStudio it could be missed by someone new to R and RStudio and their experience of not finding help might be incorrectly associated with the package.

[i] The reference to data/metadata.rda might not make sense as packages are often just used through the library() so perhaps better to use refer to the code mapmetadata::metadata?

[j] For Using a custom domain list input where domain file inputs are detailed as automatically appending so do not include these in your domain list is this because they get replicated? It would be helpful to be explicit that this wouldn't be likely to break code but just duplicate.

[k] For Using a custom lookup table input (advanced) I couldn't work out the corresponding example csv that the instruction ensure that all domain codes in the lookup file are also included in your *domain file* without opening all the csvs and looking for the column name. It might be good to expand on this with an example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant