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

Improve user interaction & error catching for prompts #80

Merged
merged 24 commits into from
Apr 10, 2024
Merged

Conversation

RayStick
Copy link
Member

@RayStick RayStick commented Apr 4, 2024

Closes #74

  • Order of README has been updated and a different json file is now used in the demo, as it can demonstrate more functionality.

Closes #62

  • this commit (7ab2f22) asks the user which tables they want to process in this session
  • improved wording with this commit (4671096)

Closes #71

  • where a list of integers is requested from the user, replaced the readline function with the scan function.
  • I have used tryCatch to avoid the code crashing if the user gives the wrong input for browsing the metadata tables

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:

  • a demo run processes the first 20 data elements (variables) in the table
  • a non-demo run processes all data elements - I think this will be the most common use-case, as I am not sure how the user would know which data elements to process at this stage, considering one of the purposes of this code is to get to know the data tables! So I took away the prompt where I asked the user to put in the row numbers of the data elements they want to process. They can always code a data element '0' with no note if they want to skip over it quickly.
    • users are now told which data element they are processing out of the total "Processing data element 1 of 20"

  • Update README to reflect all the above changes

@RayStick RayStick changed the title domain-mapping-replaced Improve user prompts Apr 4, 2024
@RayStick RayStick changed the title Improve user prompts Improve user interaction & error catching for prompts Apr 4, 2024
@RayStick RayStick marked this pull request as ready for review April 9, 2024 13:31
@RayStick
Copy link
Member Author

RayStick commented Apr 9, 2024

@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:

  • Are there any user prompts you would change the wording/formatting of?
  • Do the changes to the README make sense?
  • Any obvious user features missing that would be easy to add?
  • Test the code - test different responses to questions; try to break it with the incorrect inputs

@Rainiefantasy
Copy link
Contributor

Rainiefantasy commented Apr 9, 2024

User testing notes (pt 1):

  • all wording makes sense, barring any comments on it below
  • '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
  • also not clear how to exit, so specifying some sort of set (1,3,5) and exiting may be nicer but not necessary :)
  • '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
  • The 'Processing Table 1 of 13' and 'Processing data element 19 of 20' prints for user to read are really great and super helpful
  • this should be a simple change but is an enhancement so feel free to ignore - input 'Write a note to go with this decision (or 'N'):' . 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
  • allowing for lower case (y/n) not just upper case (Y/N) as user input
  • '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
  • 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.
  • 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)
  • 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
    Please feel free to ignore any user features that are more tricky to add; some of them looked easy but may be harder than it seems so don't worry if so! 😸

@RayStick
Copy link
Member Author

RayStick commented Apr 10, 2024

(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 8338427

[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 f1ee361 (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 4353452

[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 47012b2 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] 47012b2 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 2f8e6de 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: #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 RayStick self-assigned this Apr 10, 2024
@RayStick RayStick added this to the Before 0.2.0 release milestone Apr 10, 2024
@RayStick RayStick merged commit cd6ad32 into main Apr 10, 2024
1 check passed
@RayStick
Copy link
Member Author

@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!

@RayStick RayStick deleted the improve_prompts branch April 29, 2024 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants