diff --git a/lms/djangoapps/courseware/tests/test_submitting_problems.py b/lms/djangoapps/courseware/tests/test_submitting_problems.py index 4d04204078de..d7a29b1ce210 100644 --- a/lms/djangoapps/courseware/tests/test_submitting_problems.py +++ b/lms/djangoapps/courseware/tests/test_submitting_problems.py @@ -28,7 +28,7 @@ OptionResponseXMLFactory, SchematicResponseXMLFactory ) -from xmodule.capa.tests.test_util import use_unsafe_codejail +from xmodule.capa.tests.test_util import UseUnsafeCodejail from xmodule.capa.xqueue_interface import XQueueInterface from common.djangoapps.course_modes.models import CourseMode from lms.djangoapps.courseware.models import BaseStudentModuleHistory, StudentModule @@ -811,7 +811,7 @@ def test_three_files(self, mock_xqueue_post): self.assertEqual(list(kwargs['files'].keys()), filenames.split()) -@use_unsafe_codejail() +@UseUnsafeCodejail() class TestPythonGradedResponse(TestSubmittingProblems): """ Check that we can submit a schematic and custom response, and it answers properly. diff --git a/lms/djangoapps/instructor_task/tests/test_integration.py b/lms/djangoapps/instructor_task/tests/test_integration.py index e2a2c75497b1..d7e6de32b3f4 100644 --- a/lms/djangoapps/instructor_task/tests/test_integration.py +++ b/lms/djangoapps/instructor_task/tests/test_integration.py @@ -22,7 +22,7 @@ from xmodule.capa.responsetypes import StudentInputError from xmodule.capa.tests.response_xml_factory import CodeResponseXMLFactory, CustomResponseXMLFactory -from xmodule.capa.tests.test_util import use_unsafe_codejail +from xmodule.capa.tests.test_util import UseUnsafeCodejail from lms.djangoapps.courseware.model_data import StudentModule from lms.djangoapps.grades.api import CourseGradeFactory from lms.djangoapps.instructor_task.api import ( @@ -73,7 +73,7 @@ def _assert_task_failure(self, entry_id, task_type, problem_url_name, expected_m @ddt.ddt @override_settings(RATELIMIT_ENABLE=False) -@use_unsafe_codejail() +@UseUnsafeCodejail() class TestRescoringTask(TestIntegrationTask): """ Integration-style tests for rescoring problems in a background task. diff --git a/openedx/core/djangolib/tests/test_markup.py b/openedx/core/djangolib/tests/test_markup.py index 65c47972a2fc..5801d725490e 100644 --- a/openedx/core/djangolib/tests/test_markup.py +++ b/openedx/core/djangolib/tests/test_markup.py @@ -26,11 +26,13 @@ class FormatHtmlTest(unittest.TestCase): ("нтмℓ-єѕ¢αρє∂", "<a>нтмℓ-єѕ¢αρє∂</a>"), ) def test_simple(self, before_after): + """Verify that plain text is safely HTML-escaped.""" (before, after) = before_after assert str(Text(_(before))) == after # pylint: disable=translation-of-non-string assert str(Text(before)) == after def test_formatting(self): + """Ensure Text.format correctly mixes escaped text with raw HTML.""" # The whole point of this function is to make sure this works: out = Text(_("Point & click {start}here{end}!")).format( start=HTML(""), @@ -39,6 +41,7 @@ def test_formatting(self): assert str(out) == "Point & click here!" def test_nested_formatting(self): + """Validate nested formatting where HTML contains formatted text.""" # Sometimes, you have plain text, with html inserted, and the html has # plain text inserted. It gets twisty... out = Text(_("Send {start}email{end}")).format( @@ -48,6 +51,7 @@ def test_nested_formatting(self): assert str(out) == "Send email" def test_mako(self): + """Confirm Mako templates format Text/HTML objects with expected filters.""" # The default_filters used here have to match the ones in edxmako. template = Template( """ @@ -64,6 +68,7 @@ def test_mako(self): assert out.strip() == "A & B & C" def test_ungettext(self): + """Check that ngettext output is properly formatted and HTML-escaped.""" for i in [1, 2]: out = Text(ngettext("1 & {}", "2 & {}", i)).format(HTML("<>")) assert out == f"{i} & <>" diff --git a/openedx/core/lib/safe_lxml/xmlparser.py b/openedx/core/lib/safe_lxml/xmlparser.py index 252e9ac04b29..a463ff31477e 100644 --- a/openedx/core/lib/safe_lxml/xmlparser.py +++ b/openedx/core/lib/safe_lxml/xmlparser.py @@ -22,7 +22,8 @@ class RestrictedElement(_etree.ElementBase): __slots__ = () blacklist = (_etree._Entity, _etree._ProcessingInstruction, _etree._Comment) # pylint: disable=protected-access - def _filter(self, iterator): # pylint: disable=missing-function-docstring + def _filter(self, iterator): + """Yield only elements not in the blacklist from the given iterator.""" blacklist = self.blacklist for child in iterator: if isinstance(child, blacklist): @@ -30,37 +31,37 @@ def _filter(self, iterator): # pylint: disable=missing-function-docstring yield child def __iter__(self): - iterator = super(RestrictedElement, self).__iter__() # pylint: disable=super-with-arguments + iterator = super().__iter__() return self._filter(iterator) def iterchildren(self, tag=None, reversed=False): # pylint: disable=redefined-builtin - iterator = super(RestrictedElement, self).iterchildren( # pylint: disable=super-with-arguments - tag=tag, reversed=reversed - ) + """Iterate over child elements while excluding blacklisted nodes.""" + iterator = super().iterchildren(tag=tag, reversed=reversed) return self._filter(iterator) def iter(self, tag=None, *tags): # pylint: disable=keyword-arg-before-vararg - iterator = super(RestrictedElement, self).iter(tag=tag, *tags) # pylint: disable=super-with-arguments + """Iterate over the element tree excluding blacklisted nodes.""" + iterator = super().iter(tag=tag, *tags) return self._filter(iterator) def iterdescendants(self, tag=None, *tags): # pylint: disable=keyword-arg-before-vararg - iterator = super(RestrictedElement, self).iterdescendants( # pylint: disable=super-with-arguments - tag=tag, *tags - ) + """Iterate over descendants while filtering out blacklisted nodes.""" + iterator = super().iterdescendants(tag=tag, *tags) return self._filter(iterator) def itersiblings(self, tag=None, preceding=False): - iterator = super(RestrictedElement, self).itersiblings( # pylint: disable=super-with-arguments - tag=tag, preceding=preceding - ) + """Iterate over siblings excluding blacklisted node types.""" + iterator = super().itersiblings(tag=tag, preceding=preceding) return self._filter(iterator) def getchildren(self): - iterator = super(RestrictedElement, self).__iter__() # pylint: disable=super-with-arguments + """Return a list of non-blacklisted child elements.""" + iterator = super().__iter__() return list(self._filter(iterator)) def getiterator(self, tag=None): - iterator = super(RestrictedElement, self).getiterator(tag) # pylint: disable=super-with-arguments + """Iterate over the tree with blacklisted nodes filtered out.""" + iterator = super().getiterator(tag) return self._filter(iterator) @@ -73,7 +74,8 @@ class GlobalParserTLS(threading.local): element_class = RestrictedElement - def createDefaultParser(self): # pylint: disable=missing-function-docstring + def create_default_parser(self): + """Create a secure XMLParser using the restricted element class.""" parser = _etree.XMLParser(**self.parser_config) element_class = self.element_class if self.element_class is not None: @@ -81,19 +83,21 @@ def createDefaultParser(self): # pylint: disable=missing-function-docstring parser.set_element_class_lookup(lookup) return parser - def setDefaultParser(self, parser): + def set_default_parser(self, parser): + """Store a thread-local default XML parser instance.""" self._default_parser = parser # pylint: disable=attribute-defined-outside-init - def getDefaultParser(self): # pylint: disable=missing-function-docstring + def get_default_parser(self): + """Return the thread-local default parser, creating it if missing.""" parser = getattr(self, "_default_parser", None) if parser is None: - parser = self.createDefaultParser() - self.setDefaultParser(parser) + parser = self.create_default_parser() + self.set_default_parser(parser) return parser _parser_tls = GlobalParserTLS() -getDefaultParser = _parser_tls.getDefaultParser +get_default_parser = _parser_tls.get_default_parser def check_docinfo(elementtree, forbid_dtd=False, forbid_entities=True): @@ -107,9 +111,7 @@ def check_docinfo(elementtree, forbid_dtd=False, forbid_entities=True): raise DTDForbidden(docinfo.doctype, docinfo.system_url, docinfo.public_id) if forbid_entities and not LXML3: # lxml < 3 has no iterentities() - raise NotSupportedError( - "Unable to check for entity declarations in lxml 2.x" - ) # pylint: disable=implicit-str-concat + raise NotSupportedError("Unable to check for entity declarations in lxml 2.x") if forbid_entities: for dtd in docinfo.internalDTD, docinfo.externalDTD: @@ -119,29 +121,28 @@ def check_docinfo(elementtree, forbid_dtd=False, forbid_entities=True): raise EntitiesForbidden(entity.name, entity.content, None, None, None, None) -def parse( - source, parser=None, base_url=None, forbid_dtd=False, forbid_entities=True -): # pylint: disable=missing-function-docstring +def parse(source, parser=None, base_url=None, forbid_dtd=False, forbid_entities=True): + """Securely parse XML from a source and enforce DTD/entity restrictions.""" if parser is None: - parser = getDefaultParser() + parser = get_default_parser() elementtree = _etree.parse(source, parser, base_url=base_url) check_docinfo(elementtree, forbid_dtd, forbid_entities) return elementtree -def fromstring( - text, parser=None, base_url=None, forbid_dtd=False, forbid_entities=True -): # pylint: disable=missing-function-docstring +def fromstring(text, parser=None, base_url=None, forbid_dtd=False, forbid_entities=True): + """Securely parse XML from a string and validate docinfo.""" if parser is None: - parser = getDefaultParser() + parser = get_default_parser() rootelement = _etree.fromstring(text, parser, base_url=base_url) elementtree = rootelement.getroottree() check_docinfo(elementtree, forbid_dtd, forbid_entities) return rootelement -XML = fromstring +XML = fromstring # pylint: disable=invalid-name def iterparse(*args, **kwargs): + """Disabled XML iterparse function that always raises NotSupportedError.""" raise NotSupportedError("iterparse not available") diff --git a/xmodule/capa/capa_problem.py b/xmodule/capa/capa_problem.py index 3203ad6bf1d9..6c7a0dcffa78 100644 --- a/xmodule/capa/capa_problem.py +++ b/xmodule/capa/capa_problem.py @@ -1,3 +1,4 @@ +# pylint: disable=too-many-lines # # File: capa/capa_problem.py # @@ -27,12 +28,9 @@ from lxml import etree from pytz import UTC -import xmodule.capa.customrender as customrender -import xmodule.capa.inputtypes as inputtypes -import xmodule.capa.responsetypes as responsetypes -import xmodule.capa.xqueue_interface as xqueue_interface from openedx.core.djangolib.markup import HTML, Text from openedx.core.lib.safe_lxml.xmlparser import XML +from xmodule.capa import customrender, inputtypes, responsetypes, xqueue_interface from xmodule.capa.correctmap import CorrectMap from xmodule.capa.safe_exec import safe_exec from xmodule.capa.util import contextualize_text, convert_files_to_filenames, get_course_id_from_capa_block @@ -80,7 +78,7 @@ # main class for this module -class LoncapaSystem(object): +class LoncapaSystem: # pylint: disable=too-few-public-methods,too-many-instance-attributes """ An encapsulation of resources needed from the outside. @@ -96,14 +94,14 @@ class LoncapaSystem(object): """ - def __init__( + def __init__( # pylint: disable=too-many-positional-arguments,too-many-arguments self, ajax_url, anonymous_student_id, cache, can_execute_unsafe_code, get_python_lib_zip, - DEBUG, + DEBUG, # pylint: disable=invalid-name i18n, render_template, resources_fs, @@ -126,12 +124,12 @@ def __init__( self.matlab_api_key = matlab_api_key -class LoncapaProblem(object): +class LoncapaProblem: # pylint: disable=too-many-public-methods,too-many-instance-attributes """ Main class for capa Problems. """ - def __init__( + def __init__( # pylint: disable=too-many-positional-arguments,too-many-arguments self, problem_text, id, # pylint: disable=redefined-builtin @@ -165,7 +163,7 @@ def __init__( """ - ## Initialize class variables from state + # Initialize class variables from state self.do_reset() self.problem_id = id self.capa_system = capa_system @@ -339,7 +337,7 @@ def set_initial_display(self): self.student_answers = initial_answers def __str__(self): - return "LoncapaProblem ({0})".format(self.problem_id) + return f"LoncapaProblem ({self.problem_id})" def get_state(self): """ @@ -439,7 +437,7 @@ def get_recentmost_queuetime(self): if self.correct_map.is_queued(answer_id) ] queuetimes = [ - datetime.strptime(qt_str, xqueue_interface.dateformat).replace(tzinfo=UTC) for qt_str in queuetime_strs + datetime.strptime(qt_str, xqueue_interface.DATEFORMAT).replace(tzinfo=UTC) for qt_str in queuetime_strs ] return max(queuetimes) @@ -459,7 +457,7 @@ def grade_answers(self, answers): # if answers include File objects, convert them to filenames. self.student_answers = convert_files_to_filenames(answers) new_cmap = self.get_grade_from_current_answers(answers) - self.correct_map = new_cmap # lint-amnesty, pylint: disable=attribute-defined-outside-init + self.correct_map = new_cmap # pylint: disable=attribute-defined-outside-init self.correct_map_history.append(deepcopy(new_cmap)) return self.correct_map @@ -515,7 +513,9 @@ def get_grade_from_current_answers(self, student_answers, correct_map: Optional[ # TODO: figure out where to get file submissions when rescoring. if "filesubmission" in responder.allowed_inputfields and student_answers is None: _ = self.capa_system.i18n.gettext - raise Exception(_("Cannot rescore problems with possible file submissions")) + raise Exception( # pylint: disable=broad-exception-raised + _("Cannot rescore problems with possible file submissions") + ) # use 'student_answers' only if it is provided, and if it might contain a file # submission that would not exist in the persisted "student_answers". @@ -540,7 +540,7 @@ def get_question_answers(self): """ # dict of (id, correct_answer) answer_map = {} - for response in self.responders.keys(): # lint-amnesty, pylint: disable=consider-iterating-dictionary + for response in self.responders: results = self.responder_answers[response] answer_map.update(results) @@ -560,7 +560,7 @@ def get_answer_ids(self): get_question_answers may only return a subset of these. """ answer_ids = [] - for response in self.responders.keys(): # lint-amnesty, pylint: disable=consider-iterating-dictionary + for response in self.responders: results = self.responder_answers[response] answer_ids.append(list(results.keys())) return answer_ids @@ -577,7 +577,7 @@ def find_correct_answer_text(self, answer_id): """ xml_elements = self.tree.xpath('//*[@id="' + answer_id + '"]') if not xml_elements: - return + return None xml_element = xml_elements[0] answer_text = xml_element.xpath("@answer") if answer_text: @@ -653,12 +653,12 @@ def generate_default_question_label(): # then from the first optionresponse we'll end with the

. # If we start in the second optionresponse, we'll find another response in the way, # stop early, and instead of a question we'll report "Question 2". - SKIP_ELEMS = ["description"] - LABEL_ELEMS = ["p", "label"] - while questiontext_elem is not None and questiontext_elem.tag in SKIP_ELEMS: + skip_elems = ["description"] + label_elems = ["p", "label"] + while questiontext_elem is not None and questiontext_elem.tag in skip_elems: questiontext_elem = questiontext_elem.getprevious() - if questiontext_elem is not None and questiontext_elem.tag in LABEL_ELEMS: + if questiontext_elem is not None and questiontext_elem.tag in label_elems: question_text = questiontext_elem.text else: question_text = generate_default_question_label() @@ -695,11 +695,7 @@ def find_answer_text(self, answer_id, current_answer): elif isinstance(current_answer, str) and current_answer.startswith("choice_"): # Many problem (e.g. checkbox) report "choice_0" "choice_1" etc. # Here we transform it - elems = self.tree.xpath( - '//*[@id="{answer_id}"]//*[@name="{choice_number}"]'.format( - answer_id=answer_id, choice_number=current_answer - ) - ) + elems = self.tree.xpath(f'//*[@id="{answer_id}"]//*[@name="{current_answer}"]') if len(elems) == 0: log.warning("Answer Text Missing for answer id: %s and choice number: %s", answer_id, current_answer) answer_text = "Answer Text Missing" @@ -721,7 +717,7 @@ def find_answer_text(self, answer_id, current_answer): return answer_text or "Answer Text Missing" - def do_targeted_feedback(self, tree): + def do_targeted_feedback(self, tree): # pylint: disable=too-many-locals,too-many-branches """ Implements targeted-feedback in-place on -- choice-level explanations shown to a student after submission. @@ -827,9 +823,9 @@ def handle_input_ajax(self, data): if self.inputs[input_id]: dispatch = data["dispatch"] return self.inputs[input_id].handle_ajax(dispatch, data) - else: - log.warning("Could not find matching input for id: %s", input_id) - return {} + + log.warning("Could not find matching input for id: %s", input_id) + return {} # ======= Private Methods Below ======== @@ -845,27 +841,25 @@ def _process_includes(self): try: # open using LoncapaSystem OSFS filesystem ifp = self.capa_system.resources_fs.open(filename) - except Exception as err: # lint-amnesty, pylint: disable=broad-except + except Exception as err: # pylint: disable=broad-exception-caught log.warning("Error %s in problem xml include: %s", err, etree.tostring(inc, pretty_print=True)) log.warning("Cannot find file %s in %s", filename, self.capa_system.resources_fs) # if debugging, don't fail - just log error # TODO (vshnayder): need real error handling, display to users - if not self.capa_system.DEBUG: # lint-amnesty, pylint: disable=no-else-raise + if not self.capa_system.DEBUG: raise - else: - continue + continue try: # read in and convert to XML incxml = etree.XML(ifp.read()) - except Exception as err: # lint-amnesty, pylint: disable=broad-except + except Exception as err: # pylint: disable=broad-exception-caught log.warning("Error %s in problem xml include: %s", err, etree.tostring(inc, pretty_print=True)) log.warning("Cannot parse XML in %s", (filename)) # if debugging, don't fail - just log error # TODO (vshnayder): same as above - if not self.capa_system.DEBUG: # lint-amnesty, pylint: disable=no-else-raise + if not self.capa_system.DEBUG: raise - else: - continue + continue # insert new XML into tree in place of include parent = inc.getparent() @@ -882,15 +876,15 @@ def _extract_system_path(self, script): script : ?? (TODO) """ - DEFAULT_PATH = ["code"] + default_path = ["code"] # Separate paths by :, like the system path. - raw_path = script.get("system_path", "").split(":") + DEFAULT_PATH + raw_path = script.get("system_path", "").split(":") + default_path # find additional comma-separated modules search path path = [] - for dir in raw_path: # lint-amnesty, pylint: disable=redefined-builtin + for dir in raw_path: # pylint: disable=redefined-builtin if not dir: continue @@ -936,8 +930,8 @@ def _extract_context(self, tree): if d not in python_path and os.path.exists(d): python_path.append(d) - XMLESC = {"'": "'", """: '"'} - code = unescape(script.text, XMLESC) + xmlesc = {"'": "'", """: '"'} + code = unescape(script.text, xmlesc) all_code += code extra_files = [] @@ -961,10 +955,8 @@ def _extract_context(self, tree): unsafely=self.capa_system.can_execute_unsafe_code(), ) except Exception as err: - log.exception( # lint-amnesty, pylint: disable=logging-not-lazy - "Error while execing script code: " + all_code - ) - msg = Text("Error while executing script code: %s" % str(err)) + log.exception("Error while execing script code: %s", all_code) + msg = Text(f"Error while executing script code: {err}") raise responsetypes.LoncapaProblemError(msg) # Store code source in context, along with the Python path needed to run it correctly. @@ -973,7 +965,9 @@ def _extract_context(self, tree): context["extra_files"] = extra_files or None return context - def _extract_html(self, problemtree): # private + def _extract_html( # private + self, problemtree + ): # pylint: disable=too-many-statements,too-many-locals,too-many-branches,too-many-return-statements """ Main (private) function which converts Problem XML tree to HTML. Calls itself recursively. @@ -988,14 +982,14 @@ def _extract_html(self, problemtree): # private # and we're ok leaving those behind. # BTW: etree gives us no good way to distinguish these things # other than to examine .tag to see if it's a string. :( - return + return None if problemtree.tag == "script" and problemtree.get("type") and "javascript" in problemtree.get("type"): # leave javascript intact. return deepcopy(problemtree) if problemtree.tag in html_problem_semantics: - return + return None problemid = problemtree.get("id") # my ID @@ -1116,7 +1110,7 @@ def _preprocess_problem(self, tree, minimal_init): # private for entry in inputfields: entry.attrib["response_id"] = str(response_id) entry.attrib["answer_id"] = str(answer_id) - entry.attrib["id"] = "%s_%i_%i" % (self.problem_id, response_id, answer_id) + entry.attrib["id"] = f"{self.problem_id}_{response_id}_{answer_id}" answer_id = answer_id + 1 self.response_a11y_data(response, inputfields, responsetype_id, problem_data) @@ -1133,13 +1127,11 @@ def _preprocess_problem(self, tree, minimal_init): # private # get responder answers (do this only once, since there may be a performance cost, # eg with externalresponse) self.responder_answers = {} - for response in self.responders.keys(): # lint-amnesty, pylint: disable=consider-iterating-dictionary + for response, responder in self.responders.items(): try: - self.responder_answers[response] = self.responders[response].get_answers() - except: - log.debug( - "responder %s failed to properly return get_answers()", self.responders[response] - ) # FIXME + self.responder_answers[response] = responder.get_answers() + except Exception: + log.debug("responder %s failed to properly return get_answers()", responder) # FIXME raise # ... may not be associated with any specific response; give @@ -1147,12 +1139,14 @@ def _preprocess_problem(self, tree, minimal_init): # private # TODO: We should make the namespaces consistent and unique (e.g. %s_problem_%i). solution_id = 1 for solution in tree.findall(".//solution"): - solution.attrib["id"] = "%s_solution_%i" % (self.problem_id, solution_id) + solution.attrib["id"] = f"{self.problem_id}_solution_{solution_id}" solution_id += 1 return problem_data - def response_a11y_data(self, response, inputfields, responsetype_id, problem_data): + def response_a11y_data( # pylint: disable=too-many-locals,too-many-branches + self, response, inputfields, responsetype_id, problem_data + ): """ Construct data to be used for a11y. @@ -1173,7 +1167,7 @@ def response_a11y_data(self, response, inputfields, responsetype_id, problem_dat response.set("multiple_inputtypes", "true") group_label_tag = response.find("label") group_description_tags = response.findall("description") - group_label_tag_id = "multiinput-group-label-{}".format(responsetype_id) + group_label_tag_id = f"multiinput-group-label-{responsetype_id}" group_label_tag_text = "" if group_label_tag is not None: group_label_tag.tag = "p" @@ -1184,7 +1178,7 @@ def response_a11y_data(self, response, inputfields, responsetype_id, problem_dat group_description_ids = [] for index, group_description_tag in enumerate(group_description_tags): - group_description_tag_id = "multiinput-group-description-{}-{}".format(responsetype_id, index) + group_description_tag_id = f"multiinput-group-description-{responsetype_id}-{index}" group_description_tag.tag = "p" group_description_tag.set("id", group_description_tag_id) group_description_tag.set("class", "multi-inputs-group-description question-description") @@ -1235,9 +1229,7 @@ def response_a11y_data(self, response, inputfields, responsetype_id, problem_dat description_id = 1 descriptions = OrderedDict() for description in description_tags: - descriptions["description_%s_%i" % (responsetype_id, description_id)] = HTML( - stringify_children(description) - ) + descriptions[f"description_{responsetype_id}_{description_id}"] = HTML(stringify_children(description)) response.remove(description) description_id += 1 diff --git a/xmodule/capa/checker.py b/xmodule/capa/checker.py index b5cdd5b8145b..534be4680e6d 100755 --- a/xmodule/capa/checker.py +++ b/xmodule/capa/checker.py @@ -3,7 +3,6 @@ Commandline tool for doing operations on Problems """ - import argparse import logging import sys @@ -18,9 +17,11 @@ log = logging.getLogger("capa.checker") -class DemoSystem(object): # lint-amnesty, pylint: disable=missing-class-docstring +class DemoSystem: # pylint: disable=too-few-public-methods + """Render templates using Django's template engine.""" + def __init__(self): - self.DEBUG = True + self.DEBUG = True # pylint: disable=invalid-name def render_template(self, template_filename, dictionary): """ @@ -29,7 +30,9 @@ def render_template(self, template_filename, dictionary): return get_template(template_filename).render(dictionary) -def main(): # lint-amnesty, pylint: disable=missing-function-docstring +def main(): + """Parse command-line arguments to test or display Loncapa problem files.""" + parser = argparse.ArgumentParser(description="Check Problem Files") parser.add_argument("command", choices=["test", "show"]) # Watch? Render? Open? parser.add_argument("files", nargs="+", type=argparse.FileType("r")) @@ -47,14 +50,17 @@ def main(): # lint-amnesty, pylint: disable=missing-function-docstring system = DemoSystem() for problem_file in args.files: - log.info("Opening {0}".format(problem_file.name)) + log.info("Opening %s", problem_file.name) try: - problem = LoncapaProblem( # lint-amnesty, pylint: disable=no-value-for-parameter, unexpected-keyword-arg - problem_file, "fakeid", seed=args.seed, system=system + problem = LoncapaProblem( # pylint: disable=unexpected-keyword-arg, no-value-for-parameter + problem_file, + "fakeid", + seed=args.seed, + system=system, ) - except Exception as ex: # lint-amnesty, pylint: disable=broad-except - log.error("Could not parse file {0}".format(problem_file.name)) + except Exception as ex: # pylint: disable=broad-exception-caught + log.error("Could not parse file %s", problem_file.name) log.exception(ex) continue @@ -73,7 +79,9 @@ def command_show(problem): print(problem.get_html()) -def command_test(problem): # lint-amnesty, pylint: disable=missing-function-docstring +def command_test(problem): + """Run tests on a problem while capturing and logging stdout and stderr output.""" + # We're going to trap stdout/stderr from the problems (yes, some print) old_stdout, old_stderr = sys.stdout, sys.stderr try: @@ -83,9 +91,9 @@ def command_test(problem): # lint-amnesty, pylint: disable=missing-function-doc check_that_suggested_answers_work(problem) check_that_blanks_fail(problem) - log_captured_output(sys.stdout, "captured stdout from {0}".format(problem)) - log_captured_output(sys.stderr, "captured stderr from {0}".format(problem)) - except Exception as e: # lint-amnesty, pylint: disable=broad-except + log_captured_output(sys.stdout, f"captured stdout from {problem}") + log_captured_output(sys.stderr, f"captured stderr from {problem}") + except Exception as e: # pylint: disable=broad-exception-caught log.exception(e) finally: sys.stdout, sys.stderr = old_stdout, old_stderr @@ -99,9 +107,9 @@ def check_that_blanks_fail(problem): assert all(result == "incorrect" for result in grading_results.values()) except AssertionError: log.error( - "Blank accepted as correct answer in {0} for {1}".format( - problem, [answer_id for answer_id, result in sorted(grading_results.items()) if result != "incorrect"] - ) + "Blank accepted as correct answer in %s for %s", + problem, + [answer_id for answer_id, result in sorted(grading_results.items()) if result != "incorrect"], ) @@ -124,7 +132,7 @@ def check_that_suggested_answers_work(problem): all_answer_ids = problem.get_answer_ids() all_answers = dict((answer_id, real_answers.get(answer_id, "")) for answer_id in all_answer_ids) - log.debug("Real answers: {0}".format(real_answers)) + log.debug("Real answers: %s", real_answers) if real_answers: try: real_results = dict( @@ -135,28 +143,28 @@ def check_that_suggested_answers_work(problem): log.debug(real_results) assert all(result == "correct" for answer_id, result in real_results.items()) except UndefinedVariable as uv_exc: - log.error( # lint-amnesty, pylint: disable=logging-not-lazy - 'The variable "{0}" specified in the '.format(uv_exc) - + "solution isn't recognized (is it a units measure?)." + log.error( + 'The variable "%s" specified in the solution isn\'t recognized (is it a units measure?).', + uv_exc, ) except AssertionError: - log.error("The following generated answers were not accepted for {0}:".format(problem)) + log.error("The following generated answers were not accepted for %s:", problem) for question_id, result in sorted(real_results.items()): if result != "correct": - log.error(" {0} = {1}".format(question_id, real_answers[question_id])) - except Exception as ex: # lint-amnesty, pylint: disable=broad-except - log.error("Uncaught error in {0}".format(problem)) + log.error(" %s = %s", question_id, real_answers[question_id]) + except Exception as ex: # pylint: disable=broad-exception-caught + log.error("Uncaught error in %s", problem) log.exception(ex) -def log_captured_output(output_stream, stream_name): # lint-amnesty, pylint: disable=missing-function-docstring +def log_captured_output(output_stream, stream_name): + """Log the contents of a captured output stream with header and footer markers.""" + output_stream.seek(0) output_text = output_stream.read() if output_text: - log.info( - "##### Begin {0} #####\n".format(stream_name) + output_text - ) # lint-amnesty, pylint: disable=logging-not-lazy - log.info("##### End {0} #####".format(stream_name)) + log.info("##### Begin %s #####\n%s", stream_name, output_text) + log.info("##### End %s #####", stream_name) if __name__ == "__main__": diff --git a/xmodule/capa/correctmap.py b/xmodule/capa/correctmap.py index 5cc370e072f3..69eebc7ef602 100644 --- a/xmodule/capa/correctmap.py +++ b/xmodule/capa/correctmap.py @@ -1,11 +1,15 @@ -# lint-amnesty, pylint: disable=missing-module-docstring +""" +CorrectMap: A utility class to store and manage graded responses to CAPA questions. +Provides methods to track correctness, points, messages, hints, and queue state. +""" + # ----------------------------------------------------------------------------- # class used to store graded responses to CAPA questions # # Used by responsetypes and capa_problem -class CorrectMap(object): +class CorrectMap: """ Stores map between answer_id and response evaluation result for each question in a capa problem. The response evaluation result for each answer_id includes @@ -39,8 +43,8 @@ def __iter__(self): return self.cmap.__iter__() # See the documentation for 'set_dict' for the use of kwargs - def set( # lint-amnesty, pylint: disable=missing-function-docstring - self, # lint-amnesty, pylint: disable=unused-argument + def set( # pylint: disable=too-many-positional-arguments,too-many-arguments + self, answer_id=None, correctness=None, npoints=None, @@ -49,8 +53,12 @@ def set( # lint-amnesty, pylint: disable=missing-function-docstring hintmode=None, queuestate=None, answervariable=None, - **kwargs + **kwargs, # pylint: disable=unused-argument ): + """ + Set or update the stored evaluation result for a given answer_id. + Unused kwargs are ignored for compatibility with older formats. + """ if answer_id is not None: self.cmap[answer_id] = { @@ -124,48 +132,59 @@ def is_partially_correct(self, answer_id): return None def is_queued(self, answer_id): + """Return True if the answer has a non-empty queue state.""" return answer_id in self.cmap and self.cmap[answer_id]["queuestate"] is not None def is_right_queuekey(self, answer_id, test_key): + """Return True if the queued answer matches the provided queue key.""" return self.is_queued(answer_id) and self.cmap[answer_id]["queuestate"]["key"] == test_key def get_queuetime_str(self, answer_id): + """Return the stored queue timestamp string for the given answer.""" if self.cmap[answer_id]["queuestate"]: return self.cmap[answer_id]["queuestate"]["time"] - else: - return None + + return None def get_npoints(self, answer_id): """Return the number of points for an answer, used for partial credit.""" npoints = self.get_property(answer_id, "npoints") if npoints is not None: return npoints - elif self.is_correct(answer_id): + + if self.is_correct(answer_id): return 1 + # if not correct and no points have been assigned, return 0 return 0 - def set_property(self, answer_id, property, value): # lint-amnesty, pylint: disable=redefined-builtin + def set_property(self, answer_id, prop, value): + """Set a specific property value for the given answer_id.""" if answer_id in self.cmap: - self.cmap[answer_id][property] = value + self.cmap[answer_id][prop] = value else: - self.cmap[answer_id] = {property: value} + self.cmap[answer_id] = {prop: value} - def get_property(self, answer_id, property, default=None): # lint-amnesty, pylint: disable=redefined-builtin + def get_property(self, answer_id, prop, default=None): + """Return the specified property for an answer, or a default value.""" if answer_id in self.cmap: - return self.cmap[answer_id].get(property, default) + return self.cmap[answer_id].get(prop, default) return default def get_correctness(self, answer_id): + """Return the correctness value for the given answer.""" return self.get_property(answer_id, "correctness") def get_msg(self, answer_id): + """Return the feedback message for the given answer.""" return self.get_property(answer_id, "msg", "") def get_hint(self, answer_id): + """Return the hint text associated with the given answer.""" return self.get_property(answer_id, "hint", "") def get_hintmode(self, answer_id): + """Return the hint display mode for the given answer.""" return self.get_property(answer_id, "hintmode", None) def set_hint_and_mode(self, answer_id, hint, hintmode): @@ -181,7 +200,9 @@ def update(self, other_cmap): Update this CorrectMap with the contents of another CorrectMap """ if not isinstance(other_cmap, CorrectMap): - raise Exception("CorrectMap.update called with invalid argument %s" % other_cmap) + raise Exception( # pylint: disable=broad-exception-raised + f"CorrectMap.update called with invalid argument {other_cmap}" + ) self.cmap.update(other_cmap.get_dict()) self.set_overall_message(other_cmap.get_overall_message()) diff --git a/xmodule/capa/customrender.py b/xmodule/capa/customrender.py index 621af9fef307..8a588944842a 100644 --- a/xmodule/capa/customrender.py +++ b/xmodule/capa/customrender.py @@ -8,9 +8,9 @@ import logging import re -import xml.sax.saxutils as saxutils +from xml.sax import saxutils -from django.utils import html +from django.utils import html as html_escape from lxml import etree from .registry import TagRegistry @@ -23,7 +23,11 @@ # ----------------------------------------------------------------------------- -class MathRenderer(object): # lint-amnesty, pylint: disable=missing-class-docstring +class MathRenderer: # pylint: disable=too-few-public-methods + """ + Renders tags into MathJax-compatible HTML for displaying math expressions. + """ + tags = ["math"] def __init__(self, system, xml): @@ -48,37 +52,30 @@ def __init__(self, system, xml): mtag += "inline" else: mathstr = mathstr.replace(r"\displaystyle", "") - self.mathstr = mathstr.replace("mathjaxinline]", "%s]" % mtag) + self.mathstr = mathstr.replace("mathjaxinline]", f"{mtag}]") def get_html(self): """ Return the contents of this tag, rendered to html, as an etree element. """ # TODO: why are there nested html tags here?? Why are there html tags at all, in fact? - # xss-lint: disable=python-interpolate-html - html = "%s%s" % ( # lint-amnesty, pylint: disable=redefined-outer-name - self.mathstr, - saxutils.escape(self.xml.tail), - ) + + html = f"{self.mathstr}{saxutils.escape(self.xml.tail or '')}" try: xhtml = etree.XML(html) - except Exception as err: # lint-amnesty, pylint: disable=broad-except + except Exception as err: # pylint: disable=broad-exception-caught if self.system.DEBUG: - # xss-lint: disable=python-interpolate-html - msg = '

Error %s

' % ( - str(err).replace("<", "<") # xss-lint: disable=python-custom-escape - ) - - # xss-lint: disable=python-interpolate-html - msg += "

Failed to construct math expression from 

%s

" % html.replace( - "<", "<" # xss-lint: disable=python-custom-escape + msg = ( + f"
" + f"

Error {html_escape.escape(err)}

" + f"

Failed to construct math expression from 

{html_escape.escape(html)}

" + f"
" ) - - msg += "
" log.error(msg) return etree.XML(msg) - else: - raise + + raise + return xhtml @@ -88,7 +85,7 @@ def get_html(self): # ----------------------------------------------------------------------------- -class SolutionRenderer(object): +class SolutionRenderer: # pylint: disable=too-few-public-methods """ A solution is just a ... which is given an ID, that is used for displaying an extended answer (a problem "solution") after "show answers" is pressed. @@ -103,11 +100,10 @@ def __init__(self, system, xml): self.system = system self.id = xml.get("id") - def get_html(self): # pylint: disable=missing-function-docstring + def get_html(self): + """Return the solution HTML rendered as an etree element.""" context = {"id": self.id} - html = self.system.render_template( # lint-amnesty, pylint: disable=redefined-outer-name - "solutionspan.html", context - ) + html = self.system.render_template("solutionspan.html", context) return etree.XML(html) @@ -117,7 +113,7 @@ def get_html(self): # pylint: disable=missing-function-docstring # ----------------------------------------------------------------------------- -class TargetedFeedbackRenderer(object): +class TargetedFeedbackRenderer: # pylint: disable=too-few-public-methods """ A targeted feedback is just a ... that is used for displaying an extended piece of feedback to students if they incorrectly answered a question. @@ -134,29 +130,30 @@ def get_html(self): Return the contents of this tag, rendered to html, as an etree element. """ # xss-lint: disable=python-wrap-html - html_str = '
{}
'.format( - etree.tostring(self.xml, encoding="unicode") + html_str = ( + f'
' + f'{etree.tostring(self.xml, encoding="unicode")}' + f"
" ) try: xhtml = etree.XML(html_str) - except Exception as err: # pylint: disable=broad-except + except Exception as err: # pylint: disable=broad-exception-caught if self.system.DEBUG: # xss-lint: disable=python-wrap-html - msg = """ + msg = f""" -
-

Error {err}

-

Failed to construct targeted feedback from 

{html}

-
+
+

Error {html_escape.escape(err)}

+

Failed to construct targeted feedback from 

{html_escape.escape(html_str)}

+
- """.format( - err=html.escape(err), html=html.escape(html_str) - ) + """ log.error(msg) return etree.XML(msg) - else: - raise + + raise + return xhtml @@ -166,7 +163,7 @@ def get_html(self): # ----------------------------------------------------------------------------- -class ClarificationRenderer(object): +class ClarificationRenderer: # pylint: disable=too-few-public-methods """ A clarification appears as an inline icon which reveals more information when the user hovers over it. @@ -188,9 +185,7 @@ def get_html(self): Return the contents of this tag, rendered to html, as an etree element. """ context = {"clarification": self.inner_html} - html = self.system.render_template( # lint-amnesty, pylint: disable=redefined-outer-name - "clarification.html", context - ) + html = self.system.render_template("clarification.html", context) xml = etree.XML(html) # We must include any text that was following our original ... XML node.: xml.tail = self.tail diff --git a/xmodule/capa/inputtypes.py b/xmodule/capa/inputtypes.py index 97f0d5e6f713..ee92b8e68c91 100644 --- a/xmodule/capa/inputtypes.py +++ b/xmodule/capa/inputtypes.py @@ -1,3 +1,4 @@ +# pylint: disable=too-many-lines # # File: courseware/capa/inputtypes.py # @@ -49,9 +50,9 @@ import html5lib import nh3 +import pyparsing import six from calc.preview import latex_preview -import pyparsing from chem import chemcalc from lxml import etree @@ -65,10 +66,10 @@ log = logging.getLogger(__name__) -registry = TagRegistry() # pylint: disable=invalid-name +registry = TagRegistry() -class Status(object): +class Status: """ Problem status attributes: classname, display_name, display_tooltip @@ -111,7 +112,7 @@ def __str__(self): return self._status def __repr__(self): - return "Status(%r)" % self._status + return f"Status({self._status!r})" def __eq__(self, other): return self._status == str(other) @@ -120,7 +121,7 @@ def __hash__(self): return hash(str(self)) -class Attribute(object): +class Attribute: # pylint: disable=too-few-public-methods """ Allows specifying required and optional attributes for input types. """ @@ -128,7 +129,9 @@ class Attribute(object): # want to allow default to be None, but also allow required objects _sentinel = object() - def __init__(self, name, default=_sentinel, transform=None, validate=None, render=True): + def __init__( # pylint: disable=too-many-positional-arguments,too-many-arguments + self, name, default=_sentinel, transform=None, validate=None, render=True + ): """ Define an attribute @@ -160,7 +163,7 @@ def parse_from_xml(self, element): """ val = element.get(self.name) if self.default == self._sentinel and val is None: - raise ValueError("Missing required attribute {0}.".format(self.name)) + raise ValueError(f"Missing required attribute {self.name}.") if val is None: # not required, so return default @@ -175,7 +178,7 @@ def parse_from_xml(self, element): return val -class InputTypeBase(object): +class InputTypeBase: # pylint: disable=too-many-instance-attributes """ Abstract base class for input types. """ @@ -214,7 +217,7 @@ def __init__(self, system, xml, state): self.input_id = state.get("id", xml.get("id")) if self.input_id is None: - raise ValueError("input id state is None. xml is {0}".format(etree.tostring(xml))) + raise ValueError(f"input id state is None. xml is {etree.tostring(xml)}") self.value = state.get("value", "") @@ -242,9 +245,9 @@ def __init__(self, system, xml, state): # super().__init__, and are isolated from changes to the input # constructor interface. self.setup() - except Exception as err: # lint-amnesty, pylint: disable=broad-except + except Exception as err: # pylint: disable=broad-exception-caught # Something went wrong: add xml to message, but keep the traceback - msg = "Error in xml '{x}': {err} ".format(x=etree.tostring(xml), err=str(err)) + msg = f"Error in xml '{etree.tostring(xml)}': {err} " six.reraise(Exception, Exception(msg), sys.exc_info()[2]) @classmethod @@ -285,7 +288,6 @@ def setup(self): If this method raises an exception, it will be wrapped with a message that includes the problem xml. """ - pass # lint-amnesty, pylint: disable=unnecessary-pass def handle_ajax(self, dispatch, data): """ @@ -298,7 +300,6 @@ def handle_ajax(self, dispatch, data): Output: a dictionary object that can be serialized into JSON. This will be sent back to the Javascript. """ - pass # lint-amnesty, pylint: disable=unnecessary-pass def _get_render_context(self): """ @@ -356,7 +357,7 @@ def get_html(self): Return the html for this input, as an etree element. """ if self.template is None: - raise NotImplementedError("no rendering template specified for class {0}".format(self.__class__)) + raise NotImplementedError(f"no rendering template specified for class {self.__class__}") context = self._get_render_context() @@ -366,11 +367,12 @@ def get_html(self): output = etree.XML(html) except etree.XMLSyntaxError as ex: # If `html` contains attrs with no values, like `controls` in