Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion common/lib/capa/capa/capa_problem.py
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,9 @@ def _grade_answers(self, student_answers):
results = responder.evaluate_answers(student_answers, oldcmap)
else:
results = responder.evaluate_answers(self.student_answers, oldcmap)
newcmap.update(results)

if results: # if this responder had anything to add to the correct map

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find inline comments like this very hard to read. I prefer either:

  1. Put the comment above the line:
# if this responder had anything to add to the correct map
if results:
  1. Put the comment just two spaces after the end of the line
if results:  # if this responder had anything to add to the correct map

This comment applies throughout your Python code. Could you adjust this (especially in the longer/hairier files)? I don't want to make a repetitive comment throughout your code.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

newcmap.update(results)

self.correct_map = newcmap
return newcmap
Expand Down
56 changes: 46 additions & 10 deletions common/lib/capa/capa/inputtypes.py
Original file line number Diff line number Diff line change
Expand Up @@ -371,6 +371,38 @@ class OptionInput(InputTypeBase):
template = "optioninput.html"
tags = ['optioninput']

def __init__(self, system, xml, state):
super(OptionInput, self).__init__(system, xml, state)
self._option_elements_to_attribute_string() # if the problem follows the latest schema

def _option_elements_to_attribute_string(self):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, I like these nice long comments! I wonder if this method could be titled more along the lines of "covert to pre 1.0 options". The current title seems a little zoomed in.

I don't know that we use _xyz method naming much on this project, so you I would think about that. Like anyone who reads the comment can see that this is not some general-purpose method to call at will, and then we leave it up to them.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it is customary to leave the init as the first method of a class.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point. i moved it down.

"""
Check the problem XML for the schema to which the problem adheres. If it is the expected

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you explore the option of not introducing a schema attribute? I am trying to understand if introducing a schema attribute is absolutely necessary.

If it is necessary to introduce a schema attribute, we need to get buy-in on how the versioning is being specified from a wider audience (@dmitchell).

schema, find all the option elements, create the old-style 'options' attribute string from
those elements, and insert the manufactured attribute string into the XML file. Thus, while
the XML storage format has changed this function hides that fact, making the XML look like
it did previously so no code is broken.
:return: None
"""
options_string = "("
correct_option = ''
delimiter = ''
for option_element in self.xml.xpath('//optioninput [@id="' + self.input_id + '"]/option'):
option_name = option_element.text.strip()
options_string += delimiter + "'" + option_name + "'"
delimiter = ','
if option_element.attrib['correct'] == 'True':
correct_option = option_name

options_string += ')'
option_input_elements = self.xml.xpath('//optioninput [@id="' + self.input_id + '"]')
if option_input_elements:
# in this case there will only be a single element. self.xml is actually a fragment with
# 'optioninput' as the root element and with 'id' equal to the value in the test.
option_input_element = option_input_elements[0]
option_input_element.attrib.update({'options': options_string})
option_input_element.attrib.update({'correct': correct_option})

@staticmethod
def parse_options(options):
"""
Expand Down Expand Up @@ -483,15 +515,17 @@ def extract_choices(element, i18n):
_ = i18n.ugettext

for choice in element:
if choice.tag != 'choice':
msg = u"[capa.inputtypes.extract_choices] {error_message}".format(
# Translators: '<choice>' is a tag name and should not be translated.
error_message=_("Expected a <choice> tag; got {given_tag} instead").format(
given_tag=choice.tag
if choice.tag == 'choice':
choices.append((choice.get("name"), stringify_children(choice)))
else:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just elif choice.tag != 'booleanhint': ?

if choice.tag != 'booleanhint':
msg = u"[capa.inputtypes.extract_choices] {error_message}".format(
# Translators: '<choice>' and '<booleanhint>' are tag names and should not be translated.
error_message=_("Expected a <choice> or <booleanhint> tag; got {given_tag} instead").format(
given_tag=choice.tag
)
)
)
raise Exception(msg)
choices.append((choice.get("name"), stringify_children(choice)))
raise Exception(msg)
return choices

def get_user_visible_answer(self, internal_answer):
Expand Down Expand Up @@ -1521,12 +1555,14 @@ class AnnotationInput(InputTypeBase):

template = "annotationinput.html"
tags = ['annotationinput']
debug = False
return_to_annotation = True

def setup(self):
xml = self.xml

self.debug = False # set to True to display extra debug info with input
self.return_to_annotation = True # return only works in conjunction with annotatable xmodule
self.debug = False # set to True to display extra debug info with input
self.return_to_annotation = True # return only works in conjunction with annotatable xmodule

self.title = xml.findtext('./title', 'Annotation Exercise')
self.text = xml.findtext('./text')
Expand Down
Loading