aim-rsf / browseMetadata

An R package to help a researcher browse metadata for health datasets and categorise variables based on research domains
https://aim-rsf.github.io/browseMetadata/
GNU General Public License v3.0
3 stars 1 forks source link

Improve user interaction & error catching for prompts #80

Closed RayStick closed 3 months ago

RayStick commented 4 months ago

Closes #74


Closes #62

Closes #71

Closes #75

Though I like the idea in this Issue in terms of user flexibility, I actually think it is simpler to leave this for now and return for a later enhancement.

Code in this PR has been changed so that:


RayStick commented 3 months ago

@Rainiefantasy

I realise this a very big PR - sorry about that!

The most helpful thing will be thorough user testing. If you look through the code changes that's a bonus.

User testing:

Rainiefantasy commented 3 months ago

User testing notes (pt 1):

RayStick commented 3 months ago

(REPLY) User testing notes (pt 1):

βœ… [1] allowing for lower case (y/n) not just upper case (Y/N) as user input

Good point! Changed here 833842787f1343d3632444442400cd4d4b6b03d0

βœ… [2] instead of having users specify 'N' every time, it may be simpler to press enter if no note or enter any string to write a note

I agree. Changed here f1ee36115fa18fcc30a246751fbd282790dc2bef (with accompanying README changes - including Note column in the review, in case they accidentally press enter and want to add in a note)

βœ… [3] if you repeat row number it'll ask you to repeat the classification of that row; could make the set of rows distinct so (2,2,2,3) -> (2,3)

Good point. Changed here 4353452c305f7a886e5c801308e824c0cb06209f

βœ… [4] 'Press enter to accept the auto categorisations for table CHILD or enter each row you'd like to edit:' allows you to edit categorisations outside the auto categorisations, may be helpful to be explicit here that you can edit any row and leave code as is, or, change code to only accept the rows that have been auto categorised if you want it specific

Good point. Changed here 47012b296403eb5e38f3eb0ac95f32b551076ef5 so that they can only edit the rows that are displayed on the screen

βœ… ❓ [5] Although doesn't allow invalid datatype, it allows entering of invalid row numbers - could prompt user this is invalid if not < length(variable) & type(int) or something similar, the same way you've done for data type. Can also just leave it as it repeats prompt to enter the row numbers anyway.

Can you expand on this? I thought I had coded it so that it could not proceed with invalid row numbers πŸ˜… . Do you mean telling the user why it was invalid? The commit I did to address point [4] 47012b296403eb5e38f3eb0ac95f32b551076ef5 now includes the error feedback 'The row numbers you provided are not in range' so hopefully that addresses this point

βœ… ❓ [6a] Enter each table number you want to process in this interactive session:' - can this be changed to specify the delimiter the user should use (for eg: 'Enter each table number you want to process, separated by commas, in this interactive session') and input all in one line? may be clearer

The code wants the input to be one number for each line, so no delimiter. See lines 170 - 184 of the new README which explains to the user how this works, showing how to chose one table or two tables to process. I have changed the prompt in this commit 2f8e6deb4794463187ef6cd627ccfa72ee301f4f to be 'Enter each table number you want to process in this session (one number on each line):' - is that clear enough?

βœ… ❓ [6b] also not clear how to exit, so specifying some sort of set (1,3,5) and exiting may be nicer but not necessary :)

In the README instructions it explains 'Leave the prompt on the second row blank and press enter.' and has an example showing what that looks like. But do you think there is something in the prompt language that would make that clearer to the user? Without using too many words though

βœ… [7] 'Categorise this data element into one or more domains, e.g. 5 or 5,8:' - input other than integer (and comma delimiters) are allowed here, though if it doesn't break anything its not really a problem

Yeah I haven't managed to write any error catching for this. At least it does not crash the code, but it is true it will accept any input from the user as it is always read in as a string. Made an Issue for later: https://github.com/aim-rsf/browseMetadata/issues/82

βœ… ❓ [8] I remember there being a feature to select which variables you want to categorise for each table - is this feature removed? it doesn't show up when I process the tables

Did you see the text in the PR body that is underneath the title 'Closes 75'? Is that what you mean or something else? Happy to expand if not clear!

RayStick commented 3 months ago

@Rainiefantasy see my replies above. I have merged this PR but please look through my replies to see if we need to change anything else. Thanks for your feedback!