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

Option added for controlling number of attempts #30

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

darencard
Copy link

Note: Please bear with me, as this is my first experience with a real-world pull request. Let me know if there are any questions or you have any feedback.

Motivation: The current commit does not allow users to set the number of quiz attempts allowed and the default settings are for only 1 attempt.

Overview of changes:

  • I used the existing code that facilitated the question shuffling feature to guide changes
  • QTI standard allows an arbitrary non-negative number of attempts to be specified and user can use 0 to set unlimited attempts
  • Most changes were straightforward and should be fine
  • You may want to double-check my handling of variable types in function append_quiz_allowed_attempts in text2qti/quiz.py. I know basic Python, so was not sure if how I handled this follows best practices.
  • The new feature is briefly documented in changes to README.md
  • I did some very basic testing by eliminating the option line in quiz.md test file, which set attempts=1 in default manner. Added option line to quiz.md test file set to 0, 1, 2, 5, and 100 and uploaded to Canvas to confirm unlimited, 1, 2, 5, and 100 allowed attempts, respectively.

@gpoore
Copy link
Owner

gpoore commented Sep 2, 2020

Thanks for this! It mostly looks fine from a quick glance at the code...I'll do a more detailed check before merging.

One thing I did notice is

def append_quiz_allowed_attempts(self, text: int):

This will need to be

def append_quiz_allowed_attempts(self, text: str):

because all of these sorts of functions receive literal text arguments. It is then up to the function to verify that text is valid, and convert to other data types as necessary. So there will probably need to be a try...except for conversion to an integer, with an error message if conversion fails, followed by a check for the integer being in the accepted range (as currently exists).

Since 0 attempts is special, do you think it would make sense to use "inf" or "infinity" or something similar, instead of (or in addition to) zero? Then "inf" etc. would be converted to 0 internally in creating the QTI.

@darencard
Copy link
Author

Glad you find this useful also. And yes, that makes sense as when I switched to text: int, I still got type errors from the following < 0 logical test and had to use int(text) to convert types there and make it work as desired. Meant to change back the line you flagged, but must have forgotten.

And yes, I think with 0 being special, having a 'fuzzy' option where users can use inf, infinity, or, even, unlimited instead would make sense, as you suggest.

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 this pull request may close these issues.

None yet

2 participants