From 8cf6257ccbee64dc91f94abb3d0108c21be129cd Mon Sep 17 00:00:00 2001 From: Apurv-Bajaj Date: Tue, 22 May 2018 18:15:38 +0530 Subject: [PATCH 01/36] Add local check --- scripts/custom_lint_checks.py | 407 ++++++++++++++++++++++++++++++++++ 1 file changed, 407 insertions(+) diff --git a/scripts/custom_lint_checks.py b/scripts/custom_lint_checks.py index 4950b4c19876..897538775a81 100644 --- a/scripts/custom_lint_checks.py +++ b/scripts/custom_lint_checks.py @@ -14,11 +14,14 @@ # See the License for the specific language governing permissions and # limitations under the License. +from __future__ import print_function, division, absolute_import import astroid from pylint.checkers import BaseChecker from pylint.interfaces import IAstroidChecker from pylint.interfaces import IRawChecker +from pylint.checkers import utils as checker_utils +import _check_docs_utils as utils class ExplicitKwargsChecker(BaseChecker): @@ -143,6 +146,409 @@ def process_module(self, node): 'no-break-after-hanging-indent', line=line_num + 1) +class DocstringParameterChecker(BaseChecker): + """Checker for Sphinx, Google, or Numpy style docstrings + + * Check that all function, method and constructor parameters are mentioned + in the params and types part of the docstring. Constructor parameters + can be documented in either the class docstring or ``__init__`` docstring, + but not both. + * Check that there are no naming inconsistencies between the signature and + the documentation, i.e. also report documented parameters that are missing + in the signature. This is important to find cases where parameters are + renamed only in the code, not in the documentation. + * Check that all explicitly raised exceptions in a function are documented + in the function docstring. Caught exceptions are ignored. + + Activate this checker by adding the line:: + + load-plugins=pylint.extensions.docparams + + to the ``MASTER`` section of your ``.pylintrc``. + + :param linter: linter object + :type linter: :class:`pylint.lint.PyLinter` + """ + __implements__ = IAstroidChecker + + name = 'parameter_documentation' + msgs = { + 'W9005': ('"%s" has constructor parameters documented in class and __init__', + 'multiple-constructor-doc', + 'Please remove parameter declarations in the class or constructor.'), + 'W9006': ('"%s" not documented as being raised', + 'missing-raises-doc', + 'Please document exceptions for all raised exception types.'), + 'W9008': ('Redundant returns documentation', + 'redundant-returns-doc', + 'Please remove the return/rtype documentation from this method.'), + 'W9010': ('Redundant yields documentation', + 'redundant-yields-doc', + 'Please remove the yields documentation from this method.'), + 'W9011': ('Missing return documentation', + 'missing-return-doc', + 'Please add documentation about what this method returns.', + {'old_names': [('W9007', 'missing-returns-doc')]}), + 'W9012': ('Missing return type documentation', + 'missing-return-type-doc', + 'Please document the type returned by this method.', + # we can't use the same old_name for two different warnings + # {'old_names': [('W9007', 'missing-returns-doc')]}, + ), + 'W9013': ('Missing yield documentation', + 'missing-yield-doc', + 'Please add documentation about what this generator yields.', + {'old_names': [('W9009', 'missing-yields-doc')]}), + 'W9014': ('Missing yield type documentation', + 'missing-yield-type-doc', + 'Please document the type yielded by this method.', + # we can't use the same old_name for two different warnings + # {'old_names': [('W9009', 'missing-yields-doc')]}, + ), + 'W9015': ('"%s" missing in parameter documentation', + 'missing-param-doc', + 'Please add parameter declarations for all parameters.', + {'old_names': [('W9003', 'missing-param-doc')]}), + 'W9016': ('"%s" missing in parameter type documentation', + 'missing-type-doc', + 'Please add parameter type declarations for all parameters.', + {'old_names': [('W9004', 'missing-type-doc')]}), + 'W9017': ('"%s" differing in parameter documentation', + 'differing-param-doc', + 'Please check parameter names in declarations.', + ), + 'W9018': ('"%s" differing in parameter type documentation', + 'differing-type-doc', + 'Please check parameter names in type declarations.', + ), + } + + options = (('accept-no-param-doc', + {'default': True, 'type' : 'yn', 'metavar' : '', + 'help': 'Whether to accept totally missing parameter ' + 'documentation in the docstring of a function that has ' + 'parameters.' + }), + ('accept-no-raise-doc', + {'default': True, 'type' : 'yn', 'metavar' : '', + 'help': 'Whether to accept totally missing raises ' + 'documentation in the docstring of a function that ' + 'raises an exception.' + }), + ('accept-no-return-doc', + {'default': True, 'type' : 'yn', 'metavar' : '', + 'help': 'Whether to accept totally missing return ' + 'documentation in the docstring of a function that ' + 'returns a statement.' + }), + ('accept-no-yields-doc', + {'default': True, 'type' : 'yn', 'metavar': '', + 'help': 'Whether to accept totally missing yields ' + 'documentation in the docstring of a generator.' + }), + ) + + priority = -2 + + constructor_names = {'__init__', '__new__'} + not_needed_param_in_docstring = {'self', 'cls'} + + def visit_functiondef(self, node): + """Called for function and method definitions (def). + + :param node: Node for a function or method definition in the AST + :type node: :class:`astroid.scoped_nodes.Function` + """ + node_doc = utils.docstringify(node.doc) + self.check_functiondef_params(node, node_doc) + self.check_functiondef_returns(node, node_doc) + self.check_functiondef_yields(node, node_doc) + + def check_functiondef_params(self, node, node_doc): + node_allow_no_param = None + if node.name in self.constructor_names: + class_node = checker_utils.node_frame_class(node) + if class_node is not None: + class_doc = utils.docstringify(class_node.doc) + self.check_single_constructor_params(class_doc, node_doc, class_node) + + # __init__ or class docstrings can have no parameters documented + # as long as the other documents them. + node_allow_no_param = ( + class_doc.has_params() or + class_doc.params_documented_elsewhere() or + None + ) + class_allow_no_param = ( + node_doc.has_params() or + node_doc.params_documented_elsewhere() or + None + ) + + self.check_arguments_in_docstring( + class_doc, node.args, class_node, class_allow_no_param) + + self.check_arguments_in_docstring( + node_doc, node.args, node, node_allow_no_param) + + def check_functiondef_returns(self, node, node_doc): + if not node_doc.supports_yields and node.is_generator(): + return + + return_nodes = node.nodes_of_class(astroid.Return) + if ((node_doc.has_returns() or node_doc.has_rtype()) and + not any(utils.returns_something(ret_node) for ret_node in return_nodes)): + self.add_message( + 'redundant-returns-doc', + node=node) + + def check_functiondef_yields(self, node, node_doc): + if not node_doc.supports_yields: + return + + if ((node_doc.has_yields() or node_doc.has_yields_type()) and + not node.is_generator()): + self.add_message( + 'redundant-yields-doc', + node=node) + + def visit_raise(self, node): + func_node = node.frame() + if not isinstance(func_node, astroid.FunctionDef): + return + + expected_excs = utils.possible_exc_types(node) + if not expected_excs: + return + + if not func_node.doc: + # If this is a property setter, + # the property should have the docstring instead. + property_ = utils.get_setters_property(func_node) + if property_: + func_node = property_ + + doc = utils.docstringify(func_node.doc) + if not doc.is_valid(): + if doc.doc: + self._handle_no_raise_doc(expected_excs, func_node) + return + + found_excs = doc.exceptions() + missing_excs = expected_excs - found_excs + self._add_raise_message(missing_excs, func_node) + + def visit_return(self, node): + if not utils.returns_something(node): + return + + func_node = node.frame() + if not isinstance(func_node, astroid.FunctionDef): + return + + doc = utils.docstringify(func_node.doc) + if not doc.is_valid() and self.config.accept_no_return_doc: + return + + is_property = checker_utils.decorated_with_property(func_node) + + if not (doc.has_returns() or + (doc.has_property_returns() and is_property)): + self.add_message( + 'missing-return-doc', + node=func_node + ) + + if not (doc.has_rtype() or + (doc.has_property_type() and is_property)): + self.add_message( + 'missing-return-type-doc', + node=func_node + ) + + def visit_yield(self, node): + func_node = node.frame() + if not isinstance(func_node, astroid.FunctionDef): + return + + doc = utils.docstringify(func_node.doc) + if not doc.is_valid() and self.config.accept_no_yields_doc: + return + + if doc.supports_yields: + doc_has_yields = doc.has_yields() + doc_has_yields_type = doc.has_yields_type() + else: + doc_has_yields = doc.has_returns() + doc_has_yields_type = doc.has_rtype() + + if not doc_has_yields: + self.add_message( + 'missing-yield-doc', + node=func_node + ) + + if not doc_has_yields_type: + self.add_message( + 'missing-yield-type-doc', + node=func_node + ) + + def visit_yieldfrom(self, node): + self.visit_yield(node) + + def check_arguments_in_docstring(self, doc, arguments_node, warning_node, + accept_no_param_doc=None): + """Check that all parameters in a function, method or class constructor + on the one hand and the parameters mentioned in the parameter + documentation (e.g. the Sphinx tags 'param' and 'type') on the other + hand are consistent with each other. + + * Undocumented parameters except 'self' are noticed. + * Undocumented parameter types except for 'self' and the ``*`` + and ``**`` parameters are noticed. + * Parameters mentioned in the parameter documentation that don't or no + longer exist in the function parameter list are noticed. + * If the text "For the parameters, see" or "For the other parameters, + see" (ignoring additional whitespace) is mentioned in the docstring, + missing parameter documentation is tolerated. + * If there's no Sphinx style, Google style or NumPy style parameter + documentation at all, i.e. ``:param`` is never mentioned etc., the + checker assumes that the parameters are documented in another format + and the absence is tolerated. + + :param doc: Docstring for the function, method or class. + :type doc: str + + :param arguments_node: Arguments node for the function, method or + class constructor. + :type arguments_node: :class:`astroid.scoped_nodes.Arguments` + + :param warning_node: The node to assign the warnings to + :type warning_node: :class:`astroid.scoped_nodes.Node` + + :param accept_no_param_doc: Whether or not to allow no parameters + to be documented. + If None then this value is read from the configuration. + :type accept_no_param_doc: bool or None + """ + # Tolerate missing param or type declarations if there is a link to + # another method carrying the same name. + if not doc.doc: + return + + if accept_no_param_doc is None: + accept_no_param_doc = self.config.accept_no_param_doc + tolerate_missing_params = doc.params_documented_elsewhere() + + # Collect the function arguments. + expected_argument_names = set(arg.name for arg in arguments_node.args) + expected_argument_names.update(arg.name for arg in arguments_node.kwonlyargs) + not_needed_type_in_docstring = ( + self.not_needed_param_in_docstring.copy()) + + if arguments_node.vararg is not None: + expected_argument_names.add(arguments_node.vararg) + not_needed_type_in_docstring.add(arguments_node.vararg) + if arguments_node.kwarg is not None: + expected_argument_names.add(arguments_node.kwarg) + not_needed_type_in_docstring.add(arguments_node.kwarg) + params_with_doc, params_with_type = doc.match_param_docs() + + # Tolerate no parameter documentation at all. + if (not params_with_doc and not params_with_type + and accept_no_param_doc): + tolerate_missing_params = True + + def _compare_missing_args(found_argument_names, message_id, + not_needed_names): + """Compare the found argument names with the expected ones and + generate a message if there are arguments missing. + + :param set found_argument_names: argument names found in the + docstring + + :param str message_id: pylint message id + + :param not_needed_names: names that may be omitted + :type not_needed_names: set of str + """ + if not tolerate_missing_params: + missing_argument_names = ( + (expected_argument_names - found_argument_names) + - not_needed_names) + if missing_argument_names: + self.add_message( + message_id, + args=(', '.join( + sorted(missing_argument_names)),), + node=warning_node) + + def _compare_different_args(found_argument_names, message_id, + not_needed_names): + """Compare the found argument names with the expected ones and + generate a message if there are extra arguments found. + + :param set found_argument_names: argument names found in the + docstring + + :param str message_id: pylint message id + + :param not_needed_names: names that may be omitted + :type not_needed_names: set of str + """ + differing_argument_names = ( + (expected_argument_names ^ found_argument_names) + - not_needed_names - expected_argument_names) + + if differing_argument_names: + self.add_message( + message_id, + args=(', '.join( + sorted(differing_argument_names)),), + node=warning_node) + + _compare_missing_args(params_with_doc, 'missing-param-doc', + self.not_needed_param_in_docstring) + _compare_missing_args(params_with_type, 'missing-type-doc', + not_needed_type_in_docstring) + + _compare_different_args(params_with_doc, 'differing-param-doc', + self.not_needed_param_in_docstring) + _compare_different_args(params_with_type, 'differing-type-doc', + not_needed_type_in_docstring) + + def check_single_constructor_params(self, class_doc, init_doc, class_node): + if class_doc.has_params() and init_doc.has_params(): + self.add_message( + 'multiple-constructor-doc', + args=(class_node.name,), + node=class_node) + + def _handle_no_raise_doc(self, excs, node): + if self.config.accept_no_raise_doc: + return + + self._add_raise_message(excs, node) + + def _add_raise_message(self, missing_excs, node): + """Adds a message on :param:`node` for the missing exception type. + + :param missing_excs: A list of missing exception types. + :type missing_excs: list + + :param node: The node show the message on. + :type node: astroid.node_classes.NodeNG + """ + if not missing_excs: + return + + self.add_message( + 'missing-raises-doc', + args=(', '.join(sorted(missing_excs)),), + node=node) + + def register(linter): """Registers the checker with pylint. @@ -151,3 +557,4 @@ def register(linter): """ linter.register_checker(ExplicitKwargsChecker(linter)) linter.register_checker(HangingIndentChecker(linter)) + linter.register_checker(DocstringParameterChecker(linter)) From dab556024ec53c0929c07da76c9885eed0c45a3f Mon Sep 17 00:00:00 2001 From: Apurv-Bajaj Date: Tue, 22 May 2018 22:37:40 +0530 Subject: [PATCH 02/36] Basic setup --- .pylintrc | 2 +- scripts/_check_docs_utils.py | 724 +++++++++++++++++++++++++++++++++++ utils.py | 3 + 3 files changed, 728 insertions(+), 1 deletion(-) create mode 100644 scripts/_check_docs_utils.py diff --git a/.pylintrc b/.pylintrc index 1df659ed5f96..c4f8ca9689f1 100644 --- a/.pylintrc +++ b/.pylintrc @@ -70,7 +70,7 @@ ignore-imports=yes # too-many-statements # and fix those issues. -disable=consider-using-ternary,locally-disabled,locally-enabled,logging-not-lazy,abstract-method,arguments-differ,broad-except,duplicate-code,fixme,inconsistent-return-statements,invalid-name,len-as-condition,missing-docstring,no-else-return,no-member,no-self-use,not-context-manager,redefined-variable-type,too-many-arguments,too-many-boolean-expressions,too-many-branches,too-many-instance-attributes,too-many-lines,too-many-locals,too-many-public-methods,too-many-nested-blocks,too-many-statements,wrong-import-order +disable=consider-using-ternary,locally-disabled,locally-enabled,logging-not-lazy,abstract-method,arguments-differ,broad-except,duplicate-code,fixme,inconsistent-return-statements,invalid-name,len-as-condition,missing-docstring,missing-raises-doc,no-else-return,no-member,no-self-use,not-context-manager,redefined-variable-type,redundant-returns-doc,too-many-arguments,too-many-boolean-expressions,too-many-branches,too-many-instance-attributes,too-many-lines,too-many-locals,too-many-public-methods,too-many-nested-blocks,too-many-statements,wrong-import-order [REPORTS] diff --git a/scripts/_check_docs_utils.py b/scripts/_check_docs_utils.py new file mode 100644 index 000000000000..d7d3a296a25d --- /dev/null +++ b/scripts/_check_docs_utils.py @@ -0,0 +1,724 @@ +# coding: utf-8 + +# Copyright 2018 The Oppia Authors. All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS-IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Utility methods for docstring checking.""" + +from __future__ import absolute_import, print_function + +import re + +import astroid + +from pylint.checkers import utils + + +def space_indentation(s): + """The number of leading spaces in a string + + :param str s: input string + + :rtype: int + :return: number of leading spaces + """ + return len(s) - len(s.lstrip(' ')) + + +def get_setters_property_name(node): + """Get the name of the property that the given node is a setter for. + + :param node: The node to get the property name for. + :type node: str + + :rtype: str or None + :returns: The name of the property that the node is a setter for, + or None if one could not be found. + """ + decorators = node.decorators.nodes if node.decorators else [] + for decorator in decorators: + if (isinstance(decorator, astroid.Attribute) and + decorator.attrname == "setter" and + isinstance(decorator.expr, astroid.Name)): + return decorator.expr.name + return None + + +def get_setters_property(node): + """Get the property node for the given setter node. + + :param node: The node to get the property for. + :type node: astroid.FunctionDef + + :rtype: astroid.FunctionDef or None + :returns: The node relating to the property of the given setter node, + or None if one could not be found. + """ + property_ = None + + property_name = get_setters_property_name(node) + class_node = utils.node_frame_class(node) + if property_name and class_node: + class_attrs = class_node.getattr(node.name) + for attr in class_attrs: + if utils.decorated_with_property(attr): + property_ = attr + break + + return property_ + + +def returns_something(return_node): + """Check if a return node returns a value other than None. + + :param return_node: The return node to check. + :type return_node: astroid.Return + + :rtype: bool + :return: True if the return node returns a value other than None, + False otherwise. + """ + returns = return_node.value + + if returns is None: + return False + + return not (isinstance(returns, astroid.Const) and returns.value is None) + + +def possible_exc_types(node): + """ + Gets all of the possible raised exception types for the given raise node. + + .. note:: + + Caught exception types are ignored. + + + :param node: The raise node to find exception types for. + :type node: astroid.node_classes.NodeNG + + :returns: A list of exception types possibly raised by :param:`node`. + :rtype: set(str) + """ + excs = [] + if isinstance(node.exc, astroid.Name): + inferred = utils.safe_infer(node.exc) + if inferred: + excs = [inferred.name] + elif (isinstance(node.exc, astroid.Call) and + isinstance(node.exc.func, astroid.Name)): + target = utils.safe_infer(node.exc.func) + if isinstance(target, astroid.ClassDef): + excs = [target.name] + elif isinstance(target, astroid.FunctionDef): + for ret in target.nodes_of_class(astroid.Return): + if ret.frame() != target: + # return from inner function - ignore it + continue + + val = utils.safe_infer(ret.value) + if (val and isinstance(val, (astroid.Instance, astroid.ClassDef)) + and utils.inherit_from_std_ex(val)): + excs.append(val.name) + elif node.exc is None: + handler = node.parent + while handler and not isinstance(handler, astroid.ExceptHandler): + handler = handler.parent + + if handler and handler.type: + inferred_excs = astroid.unpack_infer(handler.type) + excs = (exc.name for exc in inferred_excs + if exc is not astroid.Uninferable) + + + try: + return set(exc for exc in excs if not utils.node_ignores_exception(node, exc)) + except astroid.InferenceError: + return set() + + +def docstringify(docstring): + for docstring_type in [SphinxDocstring, EpytextDocstring, + GoogleDocstring, NumpyDocstring]: + instance = docstring_type(docstring) + if instance.is_valid(): + return instance + + return Docstring(docstring) + + +class Docstring(object): + re_for_parameters_see = re.compile(r""" + For\s+the\s+(other)?\s*parameters\s*,\s+see + """, re.X | re.S) + + supports_yields = None + """True if the docstring supports a "yield" section. + + False if the docstring uses the returns section to document generators. + """ + + # These methods are designed to be overridden + # pylint: disable=no-self-use + def __init__(self, doc): + doc = doc or "" + self.doc = doc.expandtabs() + + def is_valid(self): + return False + + def exceptions(self): + return set() + + def has_params(self): + return False + + def has_returns(self): + return False + + def has_rtype(self): + return False + + def has_property_returns(self): + return False + + def has_property_type(self): + return False + + def has_yields(self): + return False + + def has_yields_type(self): + return False + + def match_param_docs(self): + return set(), set() + + def params_documented_elsewhere(self): + return self.re_for_parameters_see.search(self.doc) is not None + + +class SphinxDocstring(Docstring): + re_type = r"[\w\.]+" + + re_simple_container_type = r""" + {type} # a container type + [\(\[] [^\n\s]+ [\)\]] # with the contents of the container + """.format(type=re_type) + + re_xref = r""" + (?::\w+:)? # optional tag + `{0}` # what to reference + """.format(re_type) + + re_param_raw = r""" + : # initial colon + (?: # Sphinx keywords + param|parameter| + arg|argument| + key|keyword + ) + \s+ # whitespace + + (?: # optional type declaration + ({type}|{container_type}) + \s+ + )? + + (\w+) # Parameter name + \s* # whitespace + : # final colon + """.format(type=re_type, container_type=re_simple_container_type) + re_param_in_docstring = re.compile(re_param_raw, re.X | re.S) + + re_type_raw = r""" + :type # Sphinx keyword + \s+ # whitespace + ({type}) # Parameter name + \s* # whitespace + : # final colon + """.format(type=re_type) + re_type_in_docstring = re.compile(re_type_raw, re.X | re.S) + + re_property_type_raw = r""" + :type: # Sphinx keyword + \s+ # whitespace + {type} # type declaration + """.format(type=re_type) + re_property_type_in_docstring = re.compile( + re_property_type_raw, re.X | re.S + ) + + re_raise_raw = r""" + : # initial colon + (?: # Sphinx keyword + raises?| + except|exception + ) + \s+ # whitespace + + (?: # type declaration + ({type}) + \s+ + )? + + (\w+) # Parameter name + \s* # whitespace + : # final colon + """.format(type=re_type) + re_raise_in_docstring = re.compile(re_raise_raw, re.X | re.S) + + re_rtype_in_docstring = re.compile(r":rtype:") + + re_returns_in_docstring = re.compile(r":returns?:") + + supports_yields = False + + def is_valid(self): + return bool(self.re_param_in_docstring.search(self.doc) or + self.re_raise_in_docstring.search(self.doc) or + self.re_rtype_in_docstring.search(self.doc) or + self.re_returns_in_docstring.search(self.doc) or + self.re_property_type_in_docstring.search(self.doc)) + + def exceptions(self): + types = set() + + for match in re.finditer(self.re_raise_in_docstring, self.doc): + raise_type = match.group(2) + types.add(raise_type) + + return types + + def has_params(self): + if not self.doc: + return False + + return self.re_param_in_docstring.search(self.doc) is not None + + def has_returns(self): + if not self.doc: + return False + + return bool(self.re_returns_in_docstring.search(self.doc)) + + def has_rtype(self): + if not self.doc: + return False + + return bool(self.re_rtype_in_docstring.search(self.doc)) + + def has_property_returns(self): + if not self.doc: + return False + + # The summary line is the return doc, + # so the first line must not be a known directive. + return not self.doc.lstrip().startswith(':') + + def has_property_type(self): + if not self.doc: + return False + + return bool(self.re_property_type_in_docstring.search(self.doc)) + + def match_param_docs(self): + params_with_doc = set() + params_with_type = set() + + for match in re.finditer(self.re_param_in_docstring, self.doc): + name = match.group(2) + params_with_doc.add(name) + param_type = match.group(1) + if param_type is not None: + params_with_type.add(name) + + params_with_type.update(re.findall(self.re_type_in_docstring, self.doc)) + return params_with_doc, params_with_type + + +class EpytextDocstring(SphinxDocstring): + """ + Epytext is similar to Sphinx. See the docs: + http://epydoc.sourceforge.net/epytext.html + http://epydoc.sourceforge.net/fields.html#fields + + It's used in PyCharm: + https://www.jetbrains.com/help/pycharm/2016.1/creating-documentation-comments.html#d848203e314 + https://www.jetbrains.com/help/pycharm/2016.1/using-docstrings-to-specify-types.html + """ + re_param_in_docstring = re.compile( + SphinxDocstring.re_param_raw.replace(':', '@', 1), + re.X | re.S) + + re_type_in_docstring = re.compile( + SphinxDocstring.re_type_raw.replace(':', '@', 1), + re.X | re.S) + + re_property_type_in_docstring = re.compile( + SphinxDocstring.re_property_type_raw.replace(':', '@', 1), + re.X | re.S) + + re_raise_in_docstring = re.compile( + SphinxDocstring.re_raise_raw.replace(':', '@', 1), + re.X | re.S) + + re_rtype_in_docstring = re.compile(r""" + @ # initial "at" symbol + (?: # Epytext keyword + rtype|returntype + ) + : # final colon + """, re.X | re.S) + + re_returns_in_docstring = re.compile(r"@returns?:") + + def has_property_returns(self): + if not self.doc: + return False + + # If this is a property docstring, the summary is the return doc. + if self.has_property_type(): + # The summary line is the return doc, + # so the first line must not be a known directive. + return not self.doc.lstrip().startswith('@') + + return False + + +class GoogleDocstring(Docstring): + re_type = SphinxDocstring.re_type + + re_xref = SphinxDocstring.re_xref + + re_container_type = r""" + (?:{type}|{xref}) # a container type + [\(\[] [^\n]+ [\)\]] # with the contents of the container + """.format(type=re_type, xref=re_xref) + + re_multiple_type = r""" + (?:{container_type}|{type}|{xref}) + (?:\s+or\s+(?:{container_type}|{type}|{xref}))* + """.format(type=re_type, xref=re_xref, container_type=re_container_type) + + _re_section_template = r""" + ^([ ]*) {0} \s*: \s*$ # Google parameter header + ( .* ) # section + """ + + re_param_section = re.compile( + _re_section_template.format(r"(?:Args|Arguments|Parameters)"), + re.X | re.S | re.M + ) + + re_keyword_param_section = re.compile( + _re_section_template.format(r"Keyword\s(?:Args|Arguments|Parameters)"), + re.X | re.S | re.M + ) + + re_param_line = re.compile(r""" + \s* \*{{0,2}}(\w+) # identifier potentially with asterisks + \s* ( [:] + \s* + ({type}|\*) + (?:,\s+optional)? + [.] )? \s* # optional type declaration + \s* (.*) # beginning of optional description + """.format( + type=re_multiple_type, + ), re.X | re.S | re.M) + + re_raise_section = re.compile( + _re_section_template.format(r"Raises"), + re.X | re.S | re.M + ) + + re_raise_line = re.compile(r""" + \s* ({type}) \s* : # identifier + \s* (.*) # beginning of optional description + """.format(type=re_type), re.X | re.S | re.M) + + re_returns_section = re.compile( + _re_section_template.format(r"Returns?"), + re.X | re.S | re.M + ) + + re_returns_line = re.compile(r""" + \s* (({type}|\*).)? # identifier + \s* (.*) # beginning of description + """.format( + type=re_multiple_type, + ), re.X | re.S | re.M) + + re_property_returns_line = re.compile(r""" + ^{type}: # indentifier + \s* (.*) # Summary line / description + """.format( + type=re_multiple_type, + ), re.X | re.S | re.M) + + re_yields_section = re.compile( + _re_section_template.format(r"Yields?"), + re.X | re.S | re.M + ) + + re_yields_line = re_returns_line + + supports_yields = True + + def is_valid(self): + return bool(self.re_param_section.search(self.doc) or + self.re_raise_section.search(self.doc) or + self.re_returns_section.search(self.doc) or + self.re_yields_section.search(self.doc) or + self.re_property_returns_line.search(self._first_line())) + + def has_params(self): + if not self.doc: + return False + + return self.re_param_section.search(self.doc) is not None + + def has_returns(self): + if not self.doc: + return False + + entries = self._parse_section(self.re_returns_section) + for entry in entries: + match = self.re_returns_line.match(entry) + if not match: + continue + + return_desc = match.group(2) + if return_desc: + return True + + return False + + def has_rtype(self): + if not self.doc: + return False + + entries = self._parse_section(self.re_returns_section) + for entry in entries: + match = self.re_returns_line.match(entry) + if not match: + continue + + return_type = match.group(1) + if return_type: + return True + + return False + + def has_property_returns(self): + # The summary line is the return doc, + # so the first line must not be a known directive. + first_line = self._first_line() + return not bool(self.re_param_section.search(first_line) or + self.re_raise_section.search(first_line) or + self.re_returns_section.search(first_line) or + self.re_yields_section.search(first_line)) + + def has_property_type(self): + if not self.doc: + return False + + return bool(self.re_property_returns_line.match(self._first_line())) + + def has_yields(self): + if not self.doc: + return False + + entries = self._parse_section(self.re_yields_section) + for entry in entries: + match = self.re_yields_line.match(entry) + if not match: + continue + + yield_desc = match.group(2) + if yield_desc: + return True + + return False + + def has_yields_type(self): + if not self.doc: + return False + + entries = self._parse_section(self.re_yields_section) + for entry in entries: + match = self.re_yields_line.match(entry) + if not match: + continue + + yield_type = match.group(1) + if yield_type: + return True + + return False + + def exceptions(self): + types = set() + + entries = self._parse_section(self.re_raise_section) + for entry in entries: + match = self.re_raise_line.match(entry) + if not match: + continue + + exc_type = match.group(1) + exc_desc = match.group(2) + if exc_desc: + types.add(exc_type) + + return types + + def match_param_docs(self): + params_with_doc = set() + params_with_type = set() + + entries = self._parse_section(self.re_param_section) + entries.extend(self._parse_section(self.re_keyword_param_section)) + for entry in entries: + match = self.re_param_line.match(entry) + if not match: + continue + + param_name = match.group(1) + param_type = match.group(2) + param_desc = match.group(3) + if param_type: + params_with_type.add(param_name) + + if param_desc: + params_with_doc.add(param_name) + + return params_with_doc, params_with_type + + def _first_line(self): + return self.doc.lstrip().split('\n', 1)[0] + + @staticmethod + def min_section_indent(section_match): + return len(section_match.group(1)) + 1 + + @staticmethod + def _is_section_header(_): + # Google parsing does not need to detect section headers, + # because it works off of indentation level only + return False + + def _parse_section(self, section_re): + section_match = section_re.search(self.doc) + if section_match is None: + return [] + + min_indentation = self.min_section_indent(section_match) + + entries = [] + entry = [] + is_first = True + for line in section_match.group(2).splitlines(): + if not line.strip(): + continue + indentation = space_indentation(line) + if indentation < min_indentation: + break + + # The first line after the header defines the minimum + # indentation. + if is_first: + min_indentation = indentation + is_first = False + + if indentation == min_indentation: + if self._is_section_header(line): + break + # Lines with minimum indentation must contain the beginning + # of a new parameter documentation. + if entry: + entries.append("\n".join(entry)) + entry = [] + + entry.append(line) + + if entry: + entries.append("\n".join(entry)) + + return entries + + +class NumpyDocstring(GoogleDocstring): + _re_section_template = r""" + ^([ ]*) {0} \s*?$ # Numpy parameters header + \s* [-=]+ \s*?$ # underline + ( .* ) # section + """ + + re_param_section = re.compile( + _re_section_template.format(r"(?:Args|Arguments|Parameters)"), + re.X | re.S | re.M + ) + + re_param_line = re.compile(r""" + \s* (\w+) # identifier + \s* : + \s* (?:({type})(?:,\s+optional)?)? # optional type declaration + \n # description starts on a new line + \s* (.*) # description + """.format( + type=GoogleDocstring.re_multiple_type, + ), re.X | re.S) + + re_raise_section = re.compile( + _re_section_template.format(r"Raises"), + re.X | re.S | re.M + ) + + re_raise_line = re.compile(r""" + \s* ({type})$ # type declaration + \s* (.*) # optional description + """.format(type=GoogleDocstring.re_type), re.X | re.S | re.M) + + re_returns_section = re.compile( + _re_section_template.format(r"Returns?"), + re.X | re.S | re.M + ) + + re_returns_line = re.compile(r""" + \s* (?:\w+\s+:\s+)? # optional name + ({type})$ # type declaration + \s* (.*) # optional description + """.format( + type=GoogleDocstring.re_multiple_type, + ), re.X | re.S | re.M) + + re_yields_section = re.compile( + _re_section_template.format(r"Yields?"), + re.X | re.S | re.M + ) + + re_yields_line = re_returns_line + + supports_yields = True + + @staticmethod + def min_section_indent(section_match): + return len(section_match.group(1)) + + @staticmethod + def _is_section_header(line): + return bool(re.match(r'\s*-+$', line)) diff --git a/utils.py b/utils.py index edb26d5c1fd8..afd7abf5aa2e 100644 --- a/utils.py +++ b/utils.py @@ -631,6 +631,9 @@ def _get_short_language_description(full_language_description): Args: full_language_description: str. Short description of the language. + + Returns: + full_language_description: str. Short description of the language. """ if ' (' not in full_language_description: return full_language_description From 1a0934213cb1cd61f51e88e0afee71afadda8528 Mon Sep 17 00:00:00 2001 From: Apurv-Bajaj Date: Tue, 22 May 2018 23:06:45 +0530 Subject: [PATCH 03/36] Fix basic lint errors --- scripts/_check_docs_utils.py | 19 ++++++++++--------- scripts/pre_commit_linter.py | 8 +++++++- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/scripts/_check_docs_utils.py b/scripts/_check_docs_utils.py index d7d3a296a25d..3cf9400aa245 100644 --- a/scripts/_check_docs_utils.py +++ b/scripts/_check_docs_utils.py @@ -98,8 +98,7 @@ def returns_something(return_node): def possible_exc_types(node): - """ - Gets all of the possible raised exception types for the given raise node. + """Gets all of the possible raised exception types for the given raise node. .. note:: @@ -125,12 +124,13 @@ def possible_exc_types(node): elif isinstance(target, astroid.FunctionDef): for ret in target.nodes_of_class(astroid.Return): if ret.frame() != target: - # return from inner function - ignore it + # return from inner function - ignore it. continue val = utils.safe_infer(ret.value) - if (val and isinstance(val, (astroid.Instance, astroid.ClassDef)) - and utils.inherit_from_std_ex(val)): + if (val and isinstance(val, ( + astroid.Instance, astroid.ClassDef)) and + utils.inherit_from_std_ex(val)): excs.append(val.name) elif node.exc is None: handler = node.parent @@ -144,7 +144,9 @@ def possible_exc_types(node): try: - return set(exc for exc in excs if not utils.node_ignores_exception(node, exc)) + return set( + exc for exc in excs if not utils.node_ignores_exception( + node, exc)) except astroid.InferenceError: return set() @@ -350,8 +352,7 @@ def match_param_docs(self): class EpytextDocstring(SphinxDocstring): - """ - Epytext is similar to Sphinx. See the docs: + """Epytext is similar to Sphinx. See the docs: http://epydoc.sourceforge.net/epytext.html http://epydoc.sourceforge.net/fields.html#fields @@ -618,7 +619,7 @@ def min_section_indent(section_match): @staticmethod def _is_section_header(_): # Google parsing does not need to detect section headers, - # because it works off of indentation level only + # because it works off of indentation level only. return False def _parse_section(self, section_re): diff --git a/scripts/pre_commit_linter.py b/scripts/pre_commit_linter.py index b75cfb6b6f4f..8597eb324ce8 100644 --- a/scripts/pre_commit_linter.py +++ b/scripts/pre_commit_linter.py @@ -821,8 +821,14 @@ def _check_docstrings(all_files): summary_messages = [] files_to_check = [ filename for filename in all_files if not - any(fnmatch.fnmatch(filename, pattern) for pattern in EXCLUDED_PATHS) + (any( + fnmatch.fnmatch(filename, pattern) for pattern in EXCLUDED_PATHS)) + and not (filename.endswith('_check_docs_utils.py')) and filename.endswith('.py')] + # The file _check_docs_utils.py has to be excluded from this check + # as it contains a number of regular expressions which trigger this + # check. The only way to do this was to exclude this from + # files_to_check. missing_period_message = ( 'There should be a period at the end of the docstring.') multiline_docstring_message = ( From 858c9fc8f83c2bf55f190ec4b61ed704c48db2d5 Mon Sep 17 00:00:00 2001 From: Apurv-Bajaj Date: Tue, 22 May 2018 23:09:38 +0530 Subject: [PATCH 04/36] lint fix --- scripts/custom_lint_checks.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/custom_lint_checks.py b/scripts/custom_lint_checks.py index 897538775a81..9ecda50a41e0 100644 --- a/scripts/custom_lint_checks.py +++ b/scripts/custom_lint_checks.py @@ -193,7 +193,7 @@ class DocstringParameterChecker(BaseChecker): 'missing-return-type-doc', 'Please document the type returned by this method.', # we can't use the same old_name for two different warnings - # {'old_names': [('W9007', 'missing-returns-doc')]}, + # {'old_names': [('W9007', 'missing-returns-doc')]}. ), 'W9013': ('Missing yield documentation', 'missing-yield-doc', @@ -203,7 +203,7 @@ class DocstringParameterChecker(BaseChecker): 'missing-yield-type-doc', 'Please document the type yielded by this method.', # we can't use the same old_name for two different warnings - # {'old_names': [('W9009', 'missing-yields-doc')]}, + # {'old_names': [('W9009', 'missing-yields-doc')]}. ), 'W9015': ('"%s" missing in parameter documentation', 'missing-param-doc', From a604851e17838ff3f376011c368dc554bb03ecae Mon Sep 17 00:00:00 2001 From: Apurv-Bajaj Date: Tue, 22 May 2018 23:31:51 +0530 Subject: [PATCH 05/36] more lint fix --- scripts/_check_docs_utils.py | 3 +- scripts/custom_lint_checks.py | 61 +++++++++++++++++++++-------------- 2 files changed, 39 insertions(+), 25 deletions(-) diff --git a/scripts/_check_docs_utils.py b/scripts/_check_docs_utils.py index 3cf9400aa245..3475109a5749 100644 --- a/scripts/_check_docs_utils.py +++ b/scripts/_check_docs_utils.py @@ -16,7 +16,8 @@ """Utility methods for docstring checking.""" -from __future__ import absolute_import, print_function +from __future__ import absolute_import +from __future__ import print_function import re diff --git a/scripts/custom_lint_checks.py b/scripts/custom_lint_checks.py index 9ecda50a41e0..42f35d67fece 100644 --- a/scripts/custom_lint_checks.py +++ b/scripts/custom_lint_checks.py @@ -14,14 +14,17 @@ # See the License for the specific language governing permissions and # limitations under the License. -from __future__ import print_function, division, absolute_import +from __future__ import absolute_import +from __future__ import division +from __future__ import print_function + +import _check_docs_utils as utils import astroid from pylint.checkers import BaseChecker +from pylint.checkers import utils as checker_utils from pylint.interfaces import IAstroidChecker from pylint.interfaces import IRawChecker -from pylint.checkers import utils as checker_utils -import _check_docs_utils as utils class ExplicitKwargsChecker(BaseChecker): @@ -173,15 +176,19 @@ class DocstringParameterChecker(BaseChecker): name = 'parameter_documentation' msgs = { - 'W9005': ('"%s" has constructor parameters documented in class and __init__', + 'W9005': ('''"%s" has constructor parameters + documented in class and __init__''', 'multiple-constructor-doc', - 'Please remove parameter declarations in the class or constructor.'), + '''Please remove parameter declarations + in the class or constructor.'''), 'W9006': ('"%s" not documented as being raised', 'missing-raises-doc', - 'Please document exceptions for all raised exception types.'), + '''Please document exceptions for + all raised exception types.'''), 'W9008': ('Redundant returns documentation', 'redundant-returns-doc', - 'Please remove the return/rtype documentation from this method.'), + '''Please remove the return/rtype + documentation from this method.'''), 'W9010': ('Redundant yields documentation', 'redundant-yields-doc', 'Please remove the yields documentation from this method.'), @@ -224,25 +231,25 @@ class DocstringParameterChecker(BaseChecker): } options = (('accept-no-param-doc', - {'default': True, 'type' : 'yn', 'metavar' : '', + {'default': True, 'type': 'yn', 'metavar': '', 'help': 'Whether to accept totally missing parameter ' - 'documentation in the docstring of a function that has ' - 'parameters.' + 'documentation in the docstring of a ' + 'function that has parameters.' }), ('accept-no-raise-doc', - {'default': True, 'type' : 'yn', 'metavar' : '', + {'default': True, 'type': 'yn', 'metavar': '', 'help': 'Whether to accept totally missing raises ' 'documentation in the docstring of a function that ' 'raises an exception.' }), ('accept-no-return-doc', - {'default': True, 'type' : 'yn', 'metavar' : '', + {'default': True, 'type': 'yn', 'metavar': '', 'help': 'Whether to accept totally missing return ' 'documentation in the docstring of a function that ' 'returns a statement.' }), ('accept-no-yields-doc', - {'default': True, 'type' : 'yn', 'metavar': '', + {'default': True, 'type': 'yn', 'metavar': '', 'help': 'Whether to accept totally missing yields ' 'documentation in the docstring of a generator.' }), @@ -270,7 +277,8 @@ def check_functiondef_params(self, node, node_doc): class_node = checker_utils.node_frame_class(node) if class_node is not None: class_doc = utils.docstringify(class_node.doc) - self.check_single_constructor_params(class_doc, node_doc, class_node) + self.check_single_constructor_params( + class_doc, node_doc, class_node) # __init__ or class docstrings can have no parameters documented # as long as the other documents them. @@ -296,8 +304,11 @@ def check_functiondef_returns(self, node, node_doc): return return_nodes = node.nodes_of_class(astroid.Return) - if ((node_doc.has_returns() or node_doc.has_rtype()) and - not any(utils.returns_something(ret_node) for ret_node in return_nodes)): + if (( + node_doc.has_returns() or node_doc.has_rtype()) and + not any( + utils.returns_something( + ret_node) for ret_node in return_nodes)): self.add_message( 'redundant-returns-doc', node=node) @@ -397,8 +408,8 @@ def visit_yield(self, node): def visit_yieldfrom(self, node): self.visit_yield(node) - def check_arguments_in_docstring(self, doc, arguments_node, warning_node, - accept_no_param_doc=None): + def check_arguments_in_docstring( + self, doc, arguments_node, warning_node, accept_no_param_doc=None): """Check that all parameters in a function, method or class constructor on the one hand and the parameters mentioned in the parameter documentation (e.g. the Sphinx tags 'param' and 'type') on the other @@ -442,8 +453,10 @@ class constructor. tolerate_missing_params = doc.params_documented_elsewhere() # Collect the function arguments. - expected_argument_names = set(arg.name for arg in arguments_node.args) - expected_argument_names.update(arg.name for arg in arguments_node.kwonlyargs) + expected_argument_names = set( + arg.name for arg in arguments_node.args) + expected_argument_names.update( + arg.name for arg in arguments_node.kwonlyargs) not_needed_type_in_docstring = ( self.not_needed_param_in_docstring.copy()) @@ -460,8 +473,8 @@ class constructor. and accept_no_param_doc): tolerate_missing_params = True - def _compare_missing_args(found_argument_names, message_id, - not_needed_names): + def _compare_missing_args( + found_argument_names, message_id, not_needed_names): """Compare the found argument names with the expected ones and generate a message if there are arguments missing. @@ -484,8 +497,8 @@ def _compare_missing_args(found_argument_names, message_id, sorted(missing_argument_names)),), node=warning_node) - def _compare_different_args(found_argument_names, message_id, - not_needed_names): + def _compare_different_args( + found_argument_names, message_id, not_needed_names): """Compare the found argument names with the expected ones and generate a message if there are extra arguments found. From d4f2f8c8f63b818cb9b12354961410832d67ae21 Mon Sep 17 00:00:00 2001 From: Apurv-Bajaj Date: Tue, 22 May 2018 23:38:36 +0530 Subject: [PATCH 06/36] more lint fix --- scripts/pre_commit_linter.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/pre_commit_linter.py b/scripts/pre_commit_linter.py index 8597eb324ce8..63e99df963b9 100644 --- a/scripts/pre_commit_linter.py +++ b/scripts/pre_commit_linter.py @@ -634,7 +634,7 @@ def _check_bad_pattern_in_file(filename, content, pattern): Args: filename: str. Name of the file. content: str. Contents of the file. - pattern: dict ( regexp(regex pattern) : pattern to match, + pattern: dict. (regexp(regex pattern) : pattern to match, message(str) : message to show if pattern matches, excluded_files(tuple(str)) : files to be excluded from matching, excluded_dirs(tuple(str)) : directories to be excluded from @@ -823,7 +823,7 @@ def _check_docstrings(all_files): filename for filename in all_files if not (any( fnmatch.fnmatch(filename, pattern) for pattern in EXCLUDED_PATHS)) - and not (filename.endswith('_check_docs_utils.py')) + and not (filename.endswith('_check_docs_utils.py')) and filename.endswith('.py')] # The file _check_docs_utils.py has to be excluded from this check # as it contains a number of regular expressions which trigger this From 493ddabe03599eabfb413d334ae17e769befb051 Mon Sep 17 00:00:00 2001 From: Apurv-Bajaj Date: Wed, 23 May 2018 01:02:34 +0530 Subject: [PATCH 07/36] lint fix core/controllers --- core/controllers/base.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/core/controllers/base.py b/core/controllers/base.py index 15d2ae156e98..f5a867088395 100755 --- a/core/controllers/base.py +++ b/core/controllers/base.py @@ -211,6 +211,9 @@ def dispatch(self): Raises: Exception: The CSRF token is missing. UnauthorizedUserException: The CSRF token is invalid. + + Returns: + None """ # If the request is to the old demo server, redirect it permanently to # the new demo server. @@ -440,7 +443,7 @@ def handle_exception(self, exception, unused_debug_mode): """Overwrites the default exception handler. Args: - exception: The exception that was thrown. + exception: exception. The exception that was thrown. unused_debug_mode: bool. True if the web application is running in debug mode. """ @@ -572,6 +575,9 @@ def is_csrf_token_valid(cls, user_id, token): Args: user_id: str. The user_id to validate the CSRF token against. token: str. The CSRF token to validate. + + Returns: + bool. """ try: parts = token.split('/') From 0a9ef4161b5fa8a746c3199f10835509eebfc12e Mon Sep 17 00:00:00 2001 From: Apurv-Bajaj Date: Wed, 23 May 2018 02:03:35 +0530 Subject: [PATCH 08/36] partial lint fix core/domain --- core/domain/acl_decorators.py | 15 +++++++++++++++ core/domain/classifier_services.py | 2 +- core/domain/email_manager.py | 9 +++++++++ core/domain/exp_domain.py | 15 ++++++++++----- core/domain/feedback_jobs_continuous.py | 2 +- core/domain/question_services.py | 6 ++++++ core/domain/stats_services.py | 2 +- scripts/_check_docs_utils.py | 2 +- 8 files changed, 44 insertions(+), 9 deletions(-) diff --git a/core/domain/acl_decorators.py b/core/domain/acl_decorators.py index 28ffcd152390..215c547bfe43 100644 --- a/core/domain/acl_decorators.py +++ b/core/domain/acl_decorators.py @@ -45,6 +45,7 @@ def test_can_play(self, exploration_id, **kwargs): Args: exploration_id: str. The exploration id. + kwargs: keyword_arguments. """ if exploration_id in feconf.DISABLED_EXPLORATION_IDS: raise self.PageNotFoundException @@ -70,6 +71,7 @@ def test_can_play(self, collection_id, **kwargs): Args: collection_id: str. The collection id. + kwargs: keyword_arguments. """ collection_rights = rights_manager.get_collection_rights( collection_id, strict=False) @@ -94,6 +96,7 @@ def test_can_download(self, exploration_id, **kwargs): Args: exploration_id: str. The exploration id. + kwargs: keyword_arguments. """ if exploration_id in feconf.DISABLED_EXPLORATION_IDS: raise base.UserFacingExceptions.PageNotFoundException @@ -144,6 +147,7 @@ def test_can_edit(self, collection_id, **kwargs): Args: collection_id: str. The collection id. + kwargs: keyword_arguments. """ if not self.user_id: raise base.UserFacingExceptions.NotLoggedInException @@ -333,6 +337,7 @@ def test_can_access(self, exploration_id, **kwargs): Args: exploration_id: str. The exploration id. + kwargs: keyword_arguments. """ if not self.user_id: raise base.UserFacingExceptions.NotLoggedInException @@ -364,6 +369,7 @@ def test_can_rate(self, exploration_id, **kwargs): Args: exploration_id: str. The exploration id. + kwargs: keyword_arguments. """ if (role_services.ACTION_RATE_ANY_PUBLIC_EXPLORATION in self.user.actions): @@ -384,6 +390,7 @@ def test_can_flag(self, exploration_id, **kwargs): Args: exploration_id: str. The exploration id. + kwargs: keyword_arguments. """ if role_services.ACTION_FLAG_EXPLORATION in self.user.actions: return handler(self, exploration_id, **kwargs) @@ -418,6 +425,7 @@ def test_can_edit(self, exploration_id, **kwargs): Args: exploration_id: str. The exploration id. + kwargs: keyword_arguments. """ if not self.user_id: raise base.UserFacingExceptions.NotLoggedInException @@ -446,6 +454,7 @@ def test_can_delete(self, exploration_id, **kwargs): Args: exploration_id: str. The exploration id. + kwargs: keyword_arguments. """ if not self.user_id: raise base.UserFacingExceptions.NotLoggedInException @@ -475,6 +484,7 @@ def test_can_suggest(self, exploration_id, **kwargs): Args: exploration_id: str. The exploration id. + kwargs: keyword_arguments. """ if (role_services.ACTION_SUGGEST_CHANGES_TO_EXPLORATION in self.user.actions): @@ -496,6 +506,8 @@ def test_can_publish(self, exploration_id, *args, **kwargs): Args: exploration_id: str. The exploration id. + args: arguments. + kwargs: keyword_arguments. """ exploration_rights = rights_manager.get_exploration_rights( exploration_id, strict=False) @@ -522,6 +534,7 @@ def test_can_publish_collection(self, collection_id, **kwargs): Args: collection_id: str. The collection id. + kwargs: keyword_arguments. """ collection_rights = rights_manager.get_collection_rights( collection_id) @@ -547,6 +560,7 @@ def test_can_unpublish_collection(self, collection_id, **kwargs): Args: collection_id: str. The collection id. + kwargs: keyword_arguments. """ collection_rights = rights_manager.get_collection_rights( collection_id) @@ -574,6 +588,7 @@ def test_can_modify(self, exploration_id, **kwargs): Args: exploration_id: str. The exploration id. + kwargs: keyword_arguments. """ exploration_rights = rights_manager.get_exploration_rights( exploration_id, strict=False) diff --git a/core/domain/classifier_services.py b/core/domain/classifier_services.py index 61a7de3ec481..18a1187ca19b 100644 --- a/core/domain/classifier_services.py +++ b/core/domain/classifier_services.py @@ -204,7 +204,7 @@ def _update_classifier_training_jobs_status(job_ids, status): """Checks for the existence of the model and then updates it. Args: - job_id: list(str). list of ID of the ClassifierTrainingJob domain + job_ids: list(str). list of ID of the ClassifierTrainingJob domain objects. status: str. The status to which the job needs to be updated. diff --git a/core/domain/email_manager.py b/core/domain/email_manager.py index 673984bc2c71..c3598e8854ff 100644 --- a/core/domain/email_manager.py +++ b/core/domain/email_manager.py @@ -224,6 +224,9 @@ def _send_email( the email. reply_to_id: str or None. The unique reply-to id used in reply-to email address sent to recipient. + + Returns: + None. """ if sender_name is None: @@ -281,6 +284,9 @@ def _send_bulk_mail( sender_name: str. The name to be shown in the "sender" field of the email. instance_id: str or None. The ID of the BulkEmailModel entity instance. + + Returns: + None. """ _require_sender_id_is_valid(intent, sender_id) @@ -928,6 +934,9 @@ def send_test_email_for_bulk_emails(tester_id, email_subject, email_body): tester_id: str. The user ID of the tester. email_subject: str. The subject of the email. email_body: str. The body of the email. + + Returns: + None. """ tester_name = user_services.get_username(tester_id) tester_email = user_services.get_email_from_user_id(tester_id) diff --git a/core/domain/exp_domain.py b/core/domain/exp_domain.py index c038e817e4cd..c356163cf317 100644 --- a/core/domain/exp_domain.py +++ b/core/domain/exp_domain.py @@ -573,6 +573,10 @@ def to_html(self, params): Raises: Exception: 'params' is not a dict. + + Returns: + str. The HTML string that results after stripping out unrecognized tags + and attributes. """ if not isinstance(params, dict): raise Exception( @@ -875,6 +879,7 @@ def validate(self, interaction, exp_param_specs_dict): exp_param_specs_dict: dict. A dict of all parameters used in the exploration. Keys are parameter names and values are ParamSpec value objects with an object type property (obj_type). + interaction: obj. The interaction object. Raises: ValidationError: One or more attributes of the AnswerGroup are @@ -1480,11 +1485,11 @@ def update_interaction_hints(self, hints_list): """Update the list of hints. Args: - hint_list: list(dict). A list of dict; each dict represents a Hint + hints_list: list(dict). A list of dict; each dict represents a Hint object. Raises: - Exception: 'hint_list' is not a list. + Exception: 'hints_list' is not a list. """ if not isinstance(hints_list, list): raise Exception( @@ -2406,8 +2411,8 @@ def rename_state(self, old_state_name, new_state_name): """Renames the given state. Args: - old_state_names: str. The old name of state to rename. - new_state_names: str. The new state name. + old_state_name: str. The old name of state to rename. + new_state_name: str. The new state name. Raises: ValueError: The old state name does not exist or the new state name @@ -2444,7 +2449,7 @@ def delete_state(self, state_name): """Deletes the given state. Args: - state_names: str. The state name to be deleted. + state_name: str. The state name to be deleted. Raises: ValueError: The state does not exist or is the initial state of the diff --git a/core/domain/feedback_jobs_continuous.py b/core/domain/feedback_jobs_continuous.py index bca3240f5371..7382d5fd8985 100644 --- a/core/domain/feedback_jobs_continuous.py +++ b/core/domain/feedback_jobs_continuous.py @@ -63,7 +63,7 @@ def _handle_incoming_event(cls, active_realtime_layer, event_type, *args): active_realtime_layer: int. The currently active realtime datastore layer. event_type: str. The event triggered by the student. - *args: Variable length argument list. The first element of *args + args: Variable length argument list. The first element of *args corresponds to the id of the exploration currently being played. """ diff --git a/core/domain/question_services.py b/core/domain/question_services.py index 3d377ae1dcec..c16dc24e081f 100644 --- a/core/domain/question_services.py +++ b/core/domain/question_services.py @@ -35,6 +35,9 @@ def _create_new_question(committer_id, question, commit_message): committer_id: str. ID of the committer. question: Question. question domain object. commit_message: str. A description of changes made to the question. + + Returns: + model_id. The ID of the model. """ model = question_models.QuestionModel.create( title=question.title, @@ -57,6 +60,9 @@ def add_question(committer_id, question): Args: committer_id: str. ID of the committer. question: Question. Question to be saved. + + Returns: + question_id. The ID of the question. """ question.validate() commit_message = ( diff --git a/core/domain/stats_services.py b/core/domain/stats_services.py index 8240128539e9..6fff6095b51a 100644 --- a/core/domain/stats_services.py +++ b/core/domain/stats_services.py @@ -97,7 +97,7 @@ def handle_stats_creation_for_new_exploration(exp_id, exp_version, state_names): Args: exp_id: str. ID of the exploration. - exp_version. int. Version of the exploration. + exp_version: int. Version of the exploration. state_names: list(str). State names of the exploration. """ state_stats_mapping = { diff --git a/scripts/_check_docs_utils.py b/scripts/_check_docs_utils.py index 3475109a5749..c83e75897191 100644 --- a/scripts/_check_docs_utils.py +++ b/scripts/_check_docs_utils.py @@ -458,7 +458,7 @@ class GoogleDocstring(Docstring): ) re_returns_line = re.compile(r""" - \s* (({type}|\*).)? # identifier + \s* (({type}|\S*).)? # identifier \s* (.*) # beginning of description """.format( type=re_multiple_type, From a68d0d7a0b7c479bbc0b30ad368da622d120f5d9 Mon Sep 17 00:00:00 2001 From: Apurv-Bajaj Date: Wed, 23 May 2018 09:27:58 +0530 Subject: [PATCH 09/36] partial lint fix core/domain() --- core/domain/acl_decorators.py | 46 +++++++++++++++++++++++++++++++++++ scripts/_check_docs_utils.py | 2 +- 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/core/domain/acl_decorators.py b/core/domain/acl_decorators.py index 215c547bfe43..a427383cfeb2 100644 --- a/core/domain/acl_decorators.py +++ b/core/domain/acl_decorators.py @@ -46,6 +46,9 @@ def test_can_play(self, exploration_id, **kwargs): Args: exploration_id: str. The exploration id. kwargs: keyword_arguments. + + Returns: + bool. """ if exploration_id in feconf.DISABLED_EXPLORATION_IDS: raise self.PageNotFoundException @@ -72,6 +75,9 @@ def test_can_play(self, collection_id, **kwargs): Args: collection_id: str. The collection id. kwargs: keyword_arguments. + + Returns: + bool. """ collection_rights = rights_manager.get_collection_rights( collection_id, strict=False) @@ -97,6 +103,9 @@ def test_can_download(self, exploration_id, **kwargs): Args: exploration_id: str. The exploration id. kwargs: keyword_arguments. + + Returns: + bool. """ if exploration_id in feconf.DISABLED_EXPLORATION_IDS: raise base.UserFacingExceptions.PageNotFoundException @@ -123,6 +132,10 @@ def test_can_view_stats(self, exploration_id, **kwargs): Args: exploration_id: str. The exploration id. + kwargs: keyword_arguments. + + Returns: + bool. """ if exploration_id in feconf.DISABLED_EXPLORATION_IDS: raise base.UserFacingExceptions.PageNotFoundException @@ -148,6 +161,9 @@ def test_can_edit(self, collection_id, **kwargs): Args: collection_id: str. The collection id. kwargs: keyword_arguments. + + Returns: + bool. """ if not self.user_id: raise base.UserFacingExceptions.NotLoggedInException @@ -338,6 +354,9 @@ def test_can_access(self, exploration_id, **kwargs): Args: exploration_id: str. The exploration id. kwargs: keyword_arguments. + + Returns: + bool. """ if not self.user_id: raise base.UserFacingExceptions.NotLoggedInException @@ -370,6 +389,9 @@ def test_can_rate(self, exploration_id, **kwargs): Args: exploration_id: str. The exploration id. kwargs: keyword_arguments. + + Returns: + bool. """ if (role_services.ACTION_RATE_ANY_PUBLIC_EXPLORATION in self.user.actions): @@ -391,6 +413,9 @@ def test_can_flag(self, exploration_id, **kwargs): Args: exploration_id: str. The exploration id. kwargs: keyword_arguments. + + Returns: + bool. """ if role_services.ACTION_FLAG_EXPLORATION in self.user.actions: return handler(self, exploration_id, **kwargs) @@ -426,6 +451,9 @@ def test_can_edit(self, exploration_id, **kwargs): Args: exploration_id: str. The exploration id. kwargs: keyword_arguments. + + Returns: + bool. """ if not self.user_id: raise base.UserFacingExceptions.NotLoggedInException @@ -455,6 +483,9 @@ def test_can_delete(self, exploration_id, **kwargs): Args: exploration_id: str. The exploration id. kwargs: keyword_arguments. + + Returns: + bool. """ if not self.user_id: raise base.UserFacingExceptions.NotLoggedInException @@ -485,6 +516,9 @@ def test_can_suggest(self, exploration_id, **kwargs): Args: exploration_id: str. The exploration id. kwargs: keyword_arguments. + + Returns: + bool. """ if (role_services.ACTION_SUGGEST_CHANGES_TO_EXPLORATION in self.user.actions): @@ -508,6 +542,9 @@ def test_can_publish(self, exploration_id, *args, **kwargs): exploration_id: str. The exploration id. args: arguments. kwargs: keyword_arguments. + + Returns: + bool. """ exploration_rights = rights_manager.get_exploration_rights( exploration_id, strict=False) @@ -535,6 +572,9 @@ def test_can_publish_collection(self, collection_id, **kwargs): Args: collection_id: str. The collection id. kwargs: keyword_arguments. + + Returns: + bool. """ collection_rights = rights_manager.get_collection_rights( collection_id) @@ -561,6 +601,9 @@ def test_can_unpublish_collection(self, collection_id, **kwargs): Args: collection_id: str. The collection id. kwargs: keyword_arguments. + + Returns: + bool. """ collection_rights = rights_manager.get_collection_rights( collection_id) @@ -589,6 +632,9 @@ def test_can_modify(self, exploration_id, **kwargs): Args: exploration_id: str. The exploration id. kwargs: keyword_arguments. + + Returns: + bool. """ exploration_rights = rights_manager.get_exploration_rights( exploration_id, strict=False) diff --git a/scripts/_check_docs_utils.py b/scripts/_check_docs_utils.py index c83e75897191..9a779f2d6cde 100644 --- a/scripts/_check_docs_utils.py +++ b/scripts/_check_docs_utils.py @@ -434,7 +434,7 @@ class GoogleDocstring(Docstring): \s* \*{{0,2}}(\w+) # identifier potentially with asterisks \s* ( [:] \s* - ({type}|\*) + ({type}|\S*) (?:,\s+optional)? [.] )? \s* # optional type declaration \s* (.*) # beginning of optional description From 190d70d199522837d6fe76d1c46577c0b24eaf3b Mon Sep 17 00:00:00 2001 From: Apurv-Bajaj Date: Wed, 23 May 2018 11:12:26 +0530 Subject: [PATCH 10/36] partial lint fix core/domain(2) --- .pylintrc | 22 +----------------- core/domain/collection_domain.py | 4 ++-- core/domain/collection_services.py | 5 ++++- core/domain/config_domain.py | 1 + core/domain/exp_domain.py | 4 ++-- core/domain/exp_services.py | 10 ++++++++- core/domain/feedback_jobs_continuous.py | 2 +- core/domain/feedback_services.py | 30 ++++++++++++------------- core/domain/html_cleaner.py | 2 +- core/domain/rights_manager.py | 2 -- core/domain/search_services.py | 8 ++++++- core/domain/stats_domain.py | 1 + core/domain/stats_jobs_continuous.py | 3 +++ core/domain/topic_services.py | 1 + core/domain/user_domain.py | 4 ++-- core/domain/user_jobs_continuous.py | 21 ++++++++--------- core/domain/user_query_services.py | 4 ++-- core/domain/user_services.py | 3 +++ 18 files changed, 66 insertions(+), 61 deletions(-) diff --git a/.pylintrc b/.pylintrc index c4f8ca9689f1..6a8b5d80a88d 100644 --- a/.pylintrc +++ b/.pylintrc @@ -50,27 +50,7 @@ ignore-imports=yes [MESSAGES CONTROL] -# TODO(sll): Consider re-enabling the following checks: -# abstract-method -# arguments-differ -# broad-except -# duplicate-code -# fixme -# missing-docstring -# no-member -# no-self-use -# redefined-variable-type -# too-many-arguments -# too-many-boolean-expressions -# too-many-branches -# too-many-instance-attributes -# too-many-lines -# too-many-locals -# too-many-public-methods -# too-many-statements -# and fix those issues. - -disable=consider-using-ternary,locally-disabled,locally-enabled,logging-not-lazy,abstract-method,arguments-differ,broad-except,duplicate-code,fixme,inconsistent-return-statements,invalid-name,len-as-condition,missing-docstring,missing-raises-doc,no-else-return,no-member,no-self-use,not-context-manager,redefined-variable-type,redundant-returns-doc,too-many-arguments,too-many-boolean-expressions,too-many-branches,too-many-instance-attributes,too-many-lines,too-many-locals,too-many-public-methods,too-many-nested-blocks,too-many-statements,wrong-import-order +disable=consider-using-ternary,locally-disabled,locally-enabled,logging-not-lazy,abstract-method,arguments-differ,broad-except,duplicate-code,fixme,inconsistent-return-statements,invalid-name,len-as-condition,missing-docstring,missing-raises-doc,multiple-constructor-doc,no-else-return,no-member,no-self-use,not-context-manager,redefined-variable-type,redundant-returns-doc,too-many-arguments,too-many-boolean-expressions,too-many-branches,too-many-instance-attributes,too-many-lines,too-many-locals,too-many-public-methods,too-many-nested-blocks,too-many-statements,wrong-import-order [REPORTS] diff --git a/core/domain/collection_domain.py b/core/domain/collection_domain.py index 33eec737bbdd..a78846edff92 100644 --- a/core/domain/collection_domain.py +++ b/core/domain/collection_domain.py @@ -748,7 +748,7 @@ def get_next_exploration_id(self, completed_exp_ids): returns None. Args: - completed_exploration_ids: list(str). List of completed exploration + completed_exp_ids: list(str). List of completed exploration ids. Returns: @@ -789,7 +789,7 @@ def is_demo_collection_id(cls, collection_id): Args: collection_id: str. The id of the collection. - Returs: + Returns: bool. True if the collection is a demo else False. """ return collection_id in feconf.DEMO_COLLECTIONS diff --git a/core/domain/collection_services.py b/core/domain/collection_services.py index 8191ea6a0b23..917e83f0e221 100644 --- a/core/domain/collection_services.py +++ b/core/domain/collection_services.py @@ -953,7 +953,7 @@ def compute_summary_of_collection(collection, contributor_id_to_add): object and return it. Args: - collection_id: str. ID of the collection. + collection: Collection. The collection object. contributor_id_to_add: str. ID of the contributor to be added to the collection summary. @@ -1092,6 +1092,9 @@ def save_new_collection_from_yaml(committer_id, yaml_content, collection_id): committer_id: str. ID of the committer. yaml_content: str. The yaml content string specifying a collection. collection_id: str. ID of the saved collection. + + Returns: + collection. The collection object. """ collection = collection_domain.Collection.from_yaml( collection_id, yaml_content) diff --git a/core/domain/config_domain.py b/core/domain/config_domain.py index e547ea0ad7d9..73c9e4ea1548 100644 --- a/core/domain/config_domain.py +++ b/core/domain/config_domain.py @@ -208,6 +208,7 @@ def init_config_property(cls, name, instance): Args: name: str. The name of the configuration property. + instance: instance. The instance of the configuration property. """ cls._config_registry[name] = instance diff --git a/core/domain/exp_domain.py b/core/domain/exp_domain.py index c356163cf317..5c21790c977f 100644 --- a/core/domain/exp_domain.py +++ b/core/domain/exp_domain.py @@ -575,8 +575,8 @@ def to_html(self, params): Exception: 'params' is not a dict. Returns: - str. The HTML string that results after stripping out unrecognized tags - and attributes. + str. The HTML string that results after stripping + out unrecognized tags and attributes. """ if not isinstance(params, dict): raise Exception( diff --git a/core/domain/exp_services.py b/core/domain/exp_services.py index 6401c6347495..2b383cf8ccdf 100644 --- a/core/domain/exp_services.py +++ b/core/domain/exp_services.py @@ -667,6 +667,10 @@ def export_states_to_yaml(exploration_id, version=None, width=80): Args: exploration_id: str. The id of the exploration whose states should be exported. + version: int or None. The version of the exploration to be returned. + If None, the latest version of the exploration is returned. + width: int. Width for the yaml representation, default value + is set to be of 80. Returns: dict. The keys are state names, and the values are YAML strings @@ -1583,6 +1587,7 @@ def get_next_page_of_all_non_private_commits( urlsafe_start_cursor: str. If this is not None, then the returned commits start from cursor location. Otherwise they start from the beginning of the list of commits. + max_age: datetime.timedelta. Returns: tuple. A 3-tuple consisting of: @@ -1938,6 +1943,9 @@ def get_exp_with_draft_applied(exp_id, user_id): Args: exp_id: str. The id of the exploration. user_id: str. The id of the user whose draft is to be applied. + + Returns: + Exploration. The exploration domain object. """ exp_user_data = user_models.ExplorationUserDataModel.get(user_id, exp_id) @@ -1974,7 +1982,7 @@ def get_state_id_mapping(exp_id, exp_version): exp_id: str. The exploration id. exp_version: int. The exploration version. - Returnes: + Returns: StateIdMapping. Domain object for state id mapping model instance. """ model = exp_models.StateIdMappingModel.get_state_id_mapping_model( diff --git a/core/domain/feedback_jobs_continuous.py b/core/domain/feedback_jobs_continuous.py index 7382d5fd8985..93dc0f4c8b81 100644 --- a/core/domain/feedback_jobs_continuous.py +++ b/core/domain/feedback_jobs_continuous.py @@ -63,7 +63,7 @@ def _handle_incoming_event(cls, active_realtime_layer, event_type, *args): active_realtime_layer: int. The currently active realtime datastore layer. event_type: str. The event triggered by the student. - args: Variable length argument list. The first element of *args + args: variable_length_argument_list. The first element of *args corresponds to the id of the exploration currently being played. """ diff --git a/core/domain/feedback_services.py b/core/domain/feedback_services.py index 80eb321a4564..0daf5e4603ac 100644 --- a/core/domain/feedback_services.py +++ b/core/domain/feedback_services.py @@ -104,7 +104,7 @@ def create_message( exploration_id: str. The exploration id the thread belongs to. thread_id: str. The thread id the message belongs to. author_id: str. The author id who creates this message. - updated_status: str, one of STATUS_CHOICES. New thread status. + updated_status: str. one of STATUS_CHOICES. New thread status. Must be supplied if this is the first message of a thread. For the rest of the thread, should exist only when the status changes. updated_subject: str. New thread subject. Must be supplied if this is @@ -187,9 +187,9 @@ def update_messages_read_by_the_user( Args: exploration_id: str. The id of the exploration. - thread_id. str. The id of the thread. + thread_id: str. The id of the thread. user_id: str. The id of the user reading the messages, - message_ids: list(int): The ids of the messages in the thread read by + message_ids: list(int). The ids of the messages in the thread read by the user. """ feedback_thread_user_model = feedback_models.FeedbackThreadUserModel.get( @@ -210,9 +210,9 @@ def add_message_id_to_read_by_list( Args: exploration_id: str. The id of the exploration. - thread_id. str. The id of the thread. + thread_id: str. The id of the thread. user_id: str. The id of the user reading the messages, - message_id: int: The id of the message. + message_id: int. The id of the message. """ feedback_thread_user_model = feedback_models.FeedbackThreadUserModel.get( user_id, exploration_id, thread_id) @@ -743,8 +743,8 @@ def _enqueue_feedback_thread_status_change_email_task( Args: user_id: str. The user to be notified. reference: FeedbackMessageReference. - old_status: str, one of STATUS_CHOICES. - new_status: str, one of STATUS_CHOICES. + old_status: str. one of STATUS_CHOICES. + new_status: str. one of STATUS_CHOICES. """ payload = { @@ -942,7 +942,7 @@ def _send_batch_emails( sent out as a batch after a short delay. Args: - recipient_list: list of str. A list of user_ids of all recipients + recipient_list: list(str). A list of user_ids of all recipients of the email. feedback_message_reference: FeedbackMessageReference. The reference to add to each email buffer. @@ -991,10 +991,10 @@ def _send_feedback_thread_status_change_emails( """Notifies the given recipients about the status change. Args: - recipient_list: list of str. A list of recipient ids. - feedback_message_reference: FeedbackMessageReference - old_status: str, one of STATUS_CHOICES - new_status: str, one of STATUS_CHOICES + recipient_list: list(str). A list of recipient ids. + feedback_message_reference: FeedbackMessageReference. + old_status: str. one of STATUS_CHOICES + new_status: str. one of STATUS_CHOICES exploration_id: str. ID of exploration that received new message. has_suggestion: bool. Whether this thread has a related learner suggestion. @@ -1015,7 +1015,7 @@ def _ensure_each_recipient_has_reply_to_id(user_ids, exploration_id, thread_id): for each user in user_ids. Args: - user_ids: list of str. A list of user_ids. + user_ids: list(str). A list of user_ids. exploration_id: str. The id of exploration used to obtain FeedbackEmailReplyToIdModel for given user. thread_id: str. The id of thread used to obtain @@ -1048,8 +1048,8 @@ def _add_message_to_email_buffer( thread_id: str. ID of thread that received new message. message_id: int. ID of new message. message_length: int. Length of the feedback message to be sent. - old_status: str, one of STATUS_CHOICES. Value of old thread status. - new_status: str, one of STATUS_CHOICES. Value of new thread status. + old_status: str. one of STATUS_CHOICES. Value of old thread status. + new_status: str. one of STATUS_CHOICES. Value of new thread status. """ thread = ( feedback_models.FeedbackThreadModel.get_by_exp_and_thread_id( diff --git a/core/domain/html_cleaner.py b/core/domain/html_cleaner.py index 0ed22ad297df..f6569cddc945 100644 --- a/core/domain/html_cleaner.py +++ b/core/domain/html_cleaner.py @@ -115,7 +115,7 @@ def get_rte_components(html_string): """Extracts the RTE components from an HTML string. Args: - html: str. An HTML string. + html_string: str. An HTML string. Returns: list(dict). A list of dictionaries, each representing an RTE component. diff --git a/core/domain/rights_manager.py b/core/domain/rights_manager.py index 6b132beae42c..1680306d492a 100644 --- a/core/domain/rights_manager.py +++ b/core/domain/rights_manager.py @@ -764,8 +764,6 @@ def _assign_role( Args: committer: UserActionsInfo. UserActionInfo object for the user who is performing the action. - activity_rights: ExplorationRightsModel|CollectionRightsModel. The - storage object for the rights of the given activity. assignee_id: str. ID of the user whose role is being changed. new_role: str. The name of the new role: One of ROLE_OWNER diff --git a/core/domain/search_services.py b/core/domain/search_services.py index 2205a7e50890..08fdc4e4d0dc 100644 --- a/core/domain/search_services.py +++ b/core/domain/search_services.py @@ -75,6 +75,9 @@ def _should_index_exploration(exp_summary): Args: exp_summary: ExpSummaryModel. ExplorationSummary domain object. + + Returns: + bool. """ rights = rights_manager.get_exploration_rights(exp_summary.id) return rights.status != rights_manager.ACTIVITY_STATUS_PRIVATE @@ -162,6 +165,9 @@ def _should_index_collection(collection): Args: collection: CollectionSummaryModel. + + Returns: + bool. """ rights = rights_manager.get_collection_rights(collection.id) return rights.status != rights_manager.ACTIVITY_STATUS_PRIVATE @@ -171,7 +177,7 @@ def search_explorations(query, limit, sort=None, cursor=None): """Searches through the available explorations. Args: - query_string: str or None. The query string to search for. + query: str or None. The query string to search for. limit: int. The maximum number of results to return. sort: str. A string indicating how to sort results. This should be a string of space separated values. Each value should start with a diff --git a/core/domain/stats_domain.py b/core/domain/stats_domain.py index a98a342dd6bd..1d9f6576abf1 100644 --- a/core/domain/stats_domain.py +++ b/core/domain/stats_domain.py @@ -1195,6 +1195,7 @@ def __init__( corresponding to the answer calculation output. state_name: str. The name of the exploration state to which the aggregated answers were submitted. + interaction_id: str. The ID of the interaction. calculation_id: str. Which calculation was performed on the given answer data. calculation_output: AnswerCalculationOutput. The output of an diff --git a/core/domain/stats_jobs_continuous.py b/core/domain/stats_jobs_continuous.py index 77c23b42c1a9..b0bcafb3d084 100644 --- a/core/domain/stats_jobs_continuous.py +++ b/core/domain/stats_jobs_continuous.py @@ -103,6 +103,9 @@ def reduce(key, stringified_values): :: stringified_values: list(str). A list of stringified_values of the submitted answers. + + Yields: + Error. """ exploration_id, exploration_version, state_name = key.split(':') diff --git a/core/domain/topic_services.py b/core/domain/topic_services.py index f759d12ac337..98ac3db7370c 100644 --- a/core/domain/topic_services.py +++ b/core/domain/topic_services.py @@ -84,6 +84,7 @@ def get_topic_rights(topic_id, strict=True): Args: topic_id: str. ID of the topic. + strict: bool. Returns: TopicRights. The rights object associated with the given topic. diff --git a/core/domain/user_domain.py b/core/domain/user_domain.py index ab12694e17be..9bd5d9129b1b 100644 --- a/core/domain/user_domain.py +++ b/core/domain/user_domain.py @@ -216,7 +216,7 @@ def insert_exploration_id_at_given_position( Args: exploration_id: str. The exploration id to be inserted into the play later list. - position_to_be_inserted: The position at which it is to be inserted. + position_to_be_inserted: int. The position at which it is to be inserted. """ self.exploration_ids.insert( position_to_be_inserted, exploration_id) @@ -237,7 +237,7 @@ def insert_collection_id_at_given_position( Args: collection_id: str. The collection id to be inserted into the play later list. - position_to_be_inserted: The position at which it is to be inserted. + position_to_be_inserted: int. The position at which it is to be inserted. """ self.collection_ids.insert(position_to_be_inserted, collection_id) diff --git a/core/domain/user_jobs_continuous.py b/core/domain/user_jobs_continuous.py index 1391e8b1ce26..ef8770eeb3c3 100644 --- a/core/domain/user_jobs_continuous.py +++ b/core/domain/user_jobs_continuous.py @@ -124,9 +124,9 @@ def _get_most_recent_activity_commits( as those from the Oppia migration bot). Args: - activity_model_cls: The storage layer object for an activity, such + activity_model_cls: storage_layer_object. The storage layer object for an activity, such as exp_models.ExplorationModel. - activity_ids_list: A list of activity IDs (such as exploration IDS) + activity_ids_list: list(str). A list of activity IDs (such as exploration IDS) for which the latest commits will be retrieved. activity_type: str. The type of activity being referenced, such as 'exploration' or 'collection'. @@ -375,7 +375,7 @@ def _handle_incoming_event(cls, active_realtime_layer, event_type, *args): triggered and the total play count is incremented. If he/she rates an exploration, an event of type `rate` is triggered and average rating of the realtime model is refreshed. - *args: + args: If event_type is 'start', then this is a 1-element list with following entry: - str. The ID of the exploration currently being played. @@ -449,13 +449,14 @@ def get_dashboard_stats(cls, user_id): Args: user_id: str. The ID of the user. - Returns a dict with the following keys: - 'total_plays': int. Number of times the user's explorations were - played. - 'num_ratings': int. Number of times the explorations have been - rated. - 'average_ratings': float. Average of average ratings across all - explorations. + Returns: + dict. with the following keys: + 'total_plays': int. Number of times the user's explorations were + played. + 'num_ratings': int. Number of times the explorations have been + rated. + 'average_ratings': float. Average of average ratings across all + explorations. """ total_plays = 0 num_ratings = 0 diff --git a/core/domain/user_query_services.py b/core/domain/user_query_services.py index 60ec4b63e627..928c551a1b48 100644 --- a/core/domain/user_query_services.py +++ b/core/domain/user_query_services.py @@ -38,8 +38,8 @@ def save_new_query_model( by user. created_fewer_than_n_exps: int. Maximum number of explorations created by user. - edited_at_least_n_exps: Minimum number of explorations edited by user. - edited_fewer_than_n_exps: Maximum number of explorations edited by user. + edited_at_least_n_exps: int|None. Minimum number of explorations edited by user. + edited_fewer_than_n_exps: int|None. Maximum number of explorations edited by user. Returns: query_id: str. ID of the UserQueryModel instance. diff --git a/core/domain/user_services.py b/core/domain/user_services.py index 4161458aca8f..6d2f06516fb9 100644 --- a/core/domain/user_services.py +++ b/core/domain/user_services.py @@ -110,6 +110,9 @@ def __init__( user last edited an exploration. profile_picture_data_url: str or None. User uploaded profile picture as a dataURI string. + default_dashboard: str|None. The dashboard the user wants. + creator_dashboard_display_pref: str. The creator dashboard preference + the user wants. user_bio: str. User-specified biography. subject_interests: list(str) or None. Subject interests specified by the user. From e5f6404647b8d0cd74c797ba20719adbb88044fb Mon Sep 17 00:00:00 2001 From: Apurv-Bajaj Date: Wed, 23 May 2018 11:37:10 +0530 Subject: [PATCH 11/36] lint fix core/domain --- core/domain/feedback_services.py | 2 +- core/domain/search_services.py | 2 +- core/domain/user_domain.py | 9 ++++++--- core/domain/user_jobs_continuous.py | 11 ++++++----- core/domain/user_query_services.py | 6 ++++-- core/domain/user_services.py | 4 ++-- 6 files changed, 20 insertions(+), 14 deletions(-) diff --git a/core/domain/feedback_services.py b/core/domain/feedback_services.py index 0daf5e4603ac..0a52d26c66e0 100644 --- a/core/domain/feedback_services.py +++ b/core/domain/feedback_services.py @@ -968,7 +968,7 @@ def _send_instant_emails( sent out immediately. Args: - recipient_list: list of str. A list of user_ids of all + recipient_list: list(str). A list of user_ids of all recipients of the email. feedback_message_reference: FeedbackMessageReference. exploration_id: str. ID of exploration that received new message. diff --git a/core/domain/search_services.py b/core/domain/search_services.py index 08fdc4e4d0dc..9dc5ec5b3c10 100644 --- a/core/domain/search_services.py +++ b/core/domain/search_services.py @@ -239,7 +239,7 @@ def search_collections(query, limit, sort=None, cursor=None): """Searches through the available collections. Args: - query_string: str or None. the query string to search for. + query: str or None. the query string to search for. limit: int. the maximum number of results to return. sort: str. This indicates how to sort results. This should be a string of space separated values. Each value should start with a '+' or a diff --git a/core/domain/user_domain.py b/core/domain/user_domain.py index 9bd5d9129b1b..9fabc96610ef 100644 --- a/core/domain/user_domain.py +++ b/core/domain/user_domain.py @@ -38,7 +38,8 @@ class UserGlobalPrefs(object): def __init__( self, can_receive_email_updates, can_receive_editor_role_email, - can_receive_feedback_message_email, can_receive_subscription_email): + can_receive_feedback_message_email, + can_receive_subscription_email): """Constructs a UserGlobalPrefs domain object. Args: @@ -216,7 +217,8 @@ def insert_exploration_id_at_given_position( Args: exploration_id: str. The exploration id to be inserted into the play later list. - position_to_be_inserted: int. The position at which it is to be inserted. + position_to_be_inserted: int. The position at which it + is to be inserted. """ self.exploration_ids.insert( position_to_be_inserted, exploration_id) @@ -237,7 +239,8 @@ def insert_collection_id_at_given_position( Args: collection_id: str. The collection id to be inserted into the play later list. - position_to_be_inserted: int. The position at which it is to be inserted. + position_to_be_inserted: int. The position at which it + is to be inserted. """ self.collection_ids.insert(position_to_be_inserted, collection_id) diff --git a/core/domain/user_jobs_continuous.py b/core/domain/user_jobs_continuous.py index ef8770eeb3c3..a632a60b9c5b 100644 --- a/core/domain/user_jobs_continuous.py +++ b/core/domain/user_jobs_continuous.py @@ -124,10 +124,11 @@ def _get_most_recent_activity_commits( as those from the Oppia migration bot). Args: - activity_model_cls: storage_layer_object. The storage layer object for an activity, such - as exp_models.ExplorationModel. - activity_ids_list: list(str). A list of activity IDs (such as exploration IDS) - for which the latest commits will be retrieved. + activity_model_cls: storage_layer_object. The storage layer + object for an activity, such as exp_models.ExplorationModel. + activity_ids_list: list(str). A list of activity IDs + (such as exploration IDS) for which + the latest commits will be retrieved. activity_type: str. The type of activity being referenced, such as 'exploration' or 'collection'. commit_type: str. This represents the activity update commit @@ -375,7 +376,7 @@ def _handle_incoming_event(cls, active_realtime_layer, event_type, *args): triggered and the total play count is incremented. If he/she rates an exploration, an event of type `rate` is triggered and average rating of the realtime model is refreshed. - args: + args: arguments. If event_type is 'start', then this is a 1-element list with following entry: - str. The ID of the exploration currently being played. diff --git a/core/domain/user_query_services.py b/core/domain/user_query_services.py index 928c551a1b48..973d31f98757 100644 --- a/core/domain/user_query_services.py +++ b/core/domain/user_query_services.py @@ -38,8 +38,10 @@ def save_new_query_model( by user. created_fewer_than_n_exps: int. Maximum number of explorations created by user. - edited_at_least_n_exps: int|None. Minimum number of explorations edited by user. - edited_fewer_than_n_exps: int|None. Maximum number of explorations edited by user. + edited_at_least_n_exps: int|None. Minimum number of + explorations edited by user. + edited_fewer_than_n_exps: int|None. Maximum number of + explorations edited by user. Returns: query_id: str. ID of the UserQueryModel instance. diff --git a/core/domain/user_services.py b/core/domain/user_services.py index 6d2f06516fb9..72f216721de2 100644 --- a/core/domain/user_services.py +++ b/core/domain/user_services.py @@ -111,8 +111,8 @@ def __init__( profile_picture_data_url: str or None. User uploaded profile picture as a dataURI string. default_dashboard: str|None. The dashboard the user wants. - creator_dashboard_display_pref: str. The creator dashboard preference - the user wants. + creator_dashboard_display_pref: str. The creator dashboard + preference the user wants. user_bio: str. User-specified biography. subject_interests: list(str) or None. Subject interests specified by the user. From d2f2217e752b9eb873240ed3d60892304f17ea09 Mon Sep 17 00:00:00 2001 From: Apurv-Bajaj Date: Wed, 23 May 2018 11:47:29 +0530 Subject: [PATCH 12/36] lint fix core/platform --- core/platform/email/gae_email_services.py | 3 ++- core/platform/transactions/gae_transaction_services.py | 7 +++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/core/platform/email/gae_email_services.py b/core/platform/email/gae_email_services.py index 689365642e2a..0c7920fab6dc 100644 --- a/core/platform/email/gae_email_services.py +++ b/core/platform/email/gae_email_services.py @@ -92,7 +92,8 @@ def send_bulk_mail( Args: sender_email: str. The email address of the sender. This should be in the form 'SENDER_NAME '. - recipient_email: str. The email address of the recipient. + recipient_emails: list(str). The list of email addresses + of the recipients. subject: str. The subject line of the email. plaintext_body: str. The plaintext body of the email. html_body: str. The HTML body of the email. Must fit in a datastore diff --git a/core/platform/transactions/gae_transaction_services.py b/core/platform/transactions/gae_transaction_services.py index 2e427f2f92fa..e5bcfdb22ba3 100644 --- a/core/platform/transactions/gae_transaction_services.py +++ b/core/platform/transactions/gae_transaction_services.py @@ -53,7 +53,10 @@ def toplevel_wrapper(*args, **kwargs): https://developers.google.com/appengine/docs/python/ndb/async#intro Args: - *args: Variable length argument list. - **kwargs: Arbitrary keyword arguments. + args: variable_length_argument_list. + kwargs: arbitrary_keyword_arguments. + + Returns: + app. The entire app toplevel. """ return ndb.toplevel(*args, **kwargs) From 0ec1b0e9ffe12dcacb646ebd011f88a59f5a2e6b Mon Sep 17 00:00:00 2001 From: Apurv-Bajaj Date: Wed, 23 May 2018 12:29:47 +0530 Subject: [PATCH 13/36] lint fix core/storage(1) --- core/storage/classifier/gae_models.py | 3 +++ core/storage/exploration/gae_models.py | 13 ++++++++++--- core/storage/feedback/gae_models.py | 5 ++++- core/storage/file/gae_models.py | 4 ++++ core/storage/statistics/gae_models.py | 13 +++++++++++-- 5 files changed, 32 insertions(+), 6 deletions(-) diff --git a/core/storage/classifier/gae_models.py b/core/storage/classifier/gae_models.py index 1e03cd58606a..38e7ee919ab9 100644 --- a/core/storage/classifier/gae_models.py +++ b/core/storage/classifier/gae_models.py @@ -290,6 +290,9 @@ def create_multi(cls, job_exploration_mappings): Args: job_exploration_mappings: list(TrainingJobExplorationMapping). The list of TrainingJobExplorationMapping Domain objects. + + Returns: + list(int). The list of mapping IDs. """ mapping_models = [] mapping_ids = [] diff --git a/core/storage/exploration/gae_models.py b/core/storage/exploration/gae_models.py index 6ab58dead9ac..28b84dba82c4 100644 --- a/core/storage/exploration/gae_models.py +++ b/core/storage/exploration/gae_models.py @@ -315,6 +315,10 @@ def _get_instance_id(cls, exp_id, exp_version): Args: exp_id: str. The exploration id whose states are mapped. exp_version: int. The version of the exploration. + + Returns: + str. A string containing exploration ID and + exploration version. """ return 'exploration-%s-%s' % (exp_id, exp_version) @@ -603,6 +607,10 @@ def _generate_instance_id(cls, exp_id, exp_version): Args: exp_id: str. The exploration id whose states are mapped. exp_version: int. The version of the exploration. + + Returns: + str. A string containing exploration ID and + exploration version. """ return '%s.%d' % (exp_id, exp_version) @@ -645,8 +653,7 @@ def get_state_id_mapping_model(cls, exp_id, exp_version): Args: exp_id: str. The exploration id. exp_version: int. The exploration version. - strict: bool. Whether to raise an error if no StateIdMappingModel - entry is found for the given exploration id and version. + Returns: StateIdMappingModel. The model retrieved from the datastore. @@ -660,7 +667,7 @@ def delete_state_id_mapping_models(cls, exp_id, exp_versions): """Removes state id mapping models present in state_id_mapping_models. Args: - exp_id: The id of the exploration. + exp_id: str. The id of the exploration. exp_versions: list(int). A list of exploration versions for which the state id mapping model is to be deleted. """ diff --git a/core/storage/feedback/gae_models.py b/core/storage/feedback/gae_models.py index 2882ef35bdc7..04a1c81b98fc 100644 --- a/core/storage/feedback/gae_models.py +++ b/core/storage/feedback/gae_models.py @@ -88,6 +88,9 @@ def put(self, update_last_updated_time=True): Args: update_last_updated_time: bool. Whether to update the last_updated_field of the thread. + + Returns: + list. """ if update_last_updated_time: self.last_updated = datetime.datetime.utcnow() @@ -192,7 +195,7 @@ def get_by_exp_and_thread_id(cls, exploration_id, thread_id): thread_id: str. ID of the thread. Returns: - FeedbackThreadModel, or None if the thread is not found or is + FeedbackThreadModel. or None if the thread is not found or is already deleted. """ return cls.get_by_id(cls.generate_full_thread_id( diff --git a/core/storage/file/gae_models.py b/core/storage/file/gae_models.py index 8b37b2eab8be..03b068552037 100644 --- a/core/storage/file/gae_models.py +++ b/core/storage/file/gae_models.py @@ -105,6 +105,8 @@ def get_model(cls, exploration_id, filepath, strict=False): Args: exploration_id: str. The id of the exploration. filepath: str. The path to the relevant file within the exploration. + strict: bool. Whether to fail noisily if no exploration with the given + id exists in the datastore. Returns: FileMetadataModel. An instance of this class, uniquely @@ -247,6 +249,8 @@ def get_model(cls, exploration_id, filepath, strict=False): Args: exploration_id: str. The id of the exploration. filepath: str. The path to the relevant file within the exploration. + strict: bool. Whether to fail noisily if no exploration with the given + id exists in the datastore. Returns: FileModel. An instance of this class, uniquely diff --git a/core/storage/statistics/gae_models.py b/core/storage/statistics/gae_models.py index d04dbd9f9756..570d91d5bd44 100644 --- a/core/storage/statistics/gae_models.py +++ b/core/storage/statistics/gae_models.py @@ -279,6 +279,9 @@ def create( name to value. play_type: str. Type of play-through. unused_version: int. Default is 1. + + Returns: + str. The ID of the entity. """ # TODO(sll): Some events currently do not have an entity ID that was # set using this method; it was randomly set instead due tg an error. @@ -496,6 +499,9 @@ def create( play_type: str. Type of play-through. client_time_spent_in_secs: float. Time since start of this state before this event occurred. + + Returns: + str. The ID of the entity. """ entity_id = cls.get_new_event_entity_id(exp_id, session_id) complete_event_entity = cls( @@ -647,6 +653,9 @@ def create( params: dict. Current parameter values, map of parameter name to value. play_type: str. Type of play-through. + + Returns: + str. The ID of the entity. """ # TODO(sll): Some events currently do not have an entity ID that was # set using this method; it was randomly set instead due to an error. @@ -1070,7 +1079,7 @@ def get_entity_id(cls, exploration_id, exploration_version): """Gets entity_id for a batch model based on given exploration state. Args: - exp_id: str. ID of the exploration currently being played. + exploration_id: str. ID of the exploration currently being played. exploration_version: int. Version of the exploration currently being played. @@ -1110,7 +1119,7 @@ def get_versions(cls, exploration_id): ExplorationAnnotationsModel for a specific exploration_id. Args: - exp_id: str. ID of the exploration currently being played. + exploration_id: str. ID of the exploration currently being played. Returns: list(int). List of versions corresponding to annotation models From 9552aa9d2aa3af260a28686ae55eaec1daeeb640 Mon Sep 17 00:00:00 2001 From: Apurv-Bajaj Date: Wed, 23 May 2018 12:35:29 +0530 Subject: [PATCH 14/36] lint fix core/storage(2) --- core/storage/file/gae_models.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/core/storage/file/gae_models.py b/core/storage/file/gae_models.py index 03b068552037..2989249882ee 100644 --- a/core/storage/file/gae_models.py +++ b/core/storage/file/gae_models.py @@ -105,8 +105,8 @@ def get_model(cls, exploration_id, filepath, strict=False): Args: exploration_id: str. The id of the exploration. filepath: str. The path to the relevant file within the exploration. - strict: bool. Whether to fail noisily if no exploration with the given - id exists in the datastore. + strict: bool. Whether to fail noisily if no exploration + with the given id exists in the datastore. Returns: FileMetadataModel. An instance of this class, uniquely @@ -249,8 +249,8 @@ def get_model(cls, exploration_id, filepath, strict=False): Args: exploration_id: str. The id of the exploration. filepath: str. The path to the relevant file within the exploration. - strict: bool. Whether to fail noisily if no exploration with the given - id exists in the datastore. + strict: bool. Whether to fail noisily if no exploration + with the given id exists in the datastore. Returns: FileModel. An instance of this class, uniquely From 967925c7baa6c45ea9add7241d778719a524b0b8 Mon Sep 17 00:00:00 2001 From: Apurv-Bajaj Date: Wed, 23 May 2018 12:53:43 +0530 Subject: [PATCH 15/36] lint fix core/tests --- core/tests/test_utils.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/core/tests/test_utils.py b/core/tests/test_utils.py index adbf2d7a78a3..881de989fbf8 100644 --- a/core/tests/test_utils.py +++ b/core/tests/test_utils.py @@ -651,6 +651,8 @@ def save_new_valid_exploration( category: str. The category this exploration belongs to. objective: str. The objective of this exploration. language_code: str. The language_code of this exploration. + end_state_name: str. The name of the end state for the exploration. + interaction_id: str. The id of the interaction. Returns: Exploration. The exploration domain object. @@ -969,6 +971,9 @@ def urlfetch_mock( urlfetch mock. The keys of this dict are strings that represent the header name and the value represents corresponding value of that header. + + Yields: + None. """ if headers is None: response_headers = {} From 5a94ba6cd37a910122ba878d1ae6e287ec698cd9 Mon Sep 17 00:00:00 2001 From: Apurv-Bajaj Date: Wed, 23 May 2018 13:03:32 +0530 Subject: [PATCH 16/36] lint fix core/jobs.py --- core/jobs.py | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/core/jobs.py b/core/jobs.py index 898a0fd4c815..7a3ee2e9b937 100644 --- a/core/jobs.py +++ b/core/jobs.py @@ -941,10 +941,14 @@ def validate(cls, unused_mapper_spec): issue a warning if "input_reader" subdicationary is not present. Args: - mapper_spec: model.MapperSpec. The MapperSpec for this InputReader. + unused_mapper_spec: model.MapperSpec. The MapperSpec + for this InputReader. Raises: BadReaderParamsError: Required parameters are missing or invalid. + + Returns: + bool. True. """ return True # TODO. @@ -1098,6 +1102,9 @@ def put(self): Raises: Exception: The current instance has an invalid realtime layer id. + + Returns: + realtime_layer. The realtime layer entity. """ if (self.realtime_layer is None or str(self.realtime_layer) != self.id[0]): @@ -1210,8 +1217,8 @@ def _handle_incoming_event( a student starts an exploration, event of type `start` is triggered. If he/she completes an exploration, event of type `complete` is triggered. - *args: list(*). Forwarded to the _handle_event() method. - **kwargs: dict(* : *). Forwarded to the _handle_event() method. + args: list(*). Forwarded to the _handle_event() method. + kwargs: dict(* : *). Forwarded to the _handle_event() method. """ raise NotImplementedError( 'Subclasses of BaseContinuousComputationManager must implement ' @@ -1439,8 +1446,8 @@ def on_incoming_event(cls, event_type, *args, **kwargs): a student starts an exploration, event of type `start` is triggered. If he/she completes an exploration, event of type `complete` is triggered. - *args: Forwarded to _handle_event() method. - *kwargs: Forwarded to _handle_event() method. + args: arguments. Forwarded to _handle_event() method. + kwargs: keyword_arguments. Forwarded to _handle_event() method. """ realtime_layers = [0, 1] for layer in realtime_layers: From 3740fa1d3fa777020b738671f4b565d03774f7f3 Mon Sep 17 00:00:00 2001 From: Apurv-Bajaj Date: Wed, 23 May 2018 13:12:37 +0530 Subject: [PATCH 17/36] lint fix scripts --- scripts/install_third_party.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/install_third_party.py b/scripts/install_third_party.py index 52bd942eb841..3c8dbbfdfa85 100644 --- a/scripts/install_third_party.py +++ b/scripts/install_third_party.py @@ -201,8 +201,8 @@ def test_manifest_syntax(dependency_type, dependency_dict): Display warning message when there is an error and terminate the program. Args: - dependency_type: dependency download format. - dependency_dict: manifest.json dependency dict. + dependency_type: dependency_download_format. + dependency_dict: dict. manifest.json dependency dict. """ keys = dependency_dict.keys() mandatory_keys = DOWNLOAD_FORMATS_TO_MANIFEST_KEYS[ From aeeb2b66e2ce73deabc75b0f15a6b1f658bd218b Mon Sep 17 00:00:00 2001 From: Apurv-Bajaj Date: Wed, 23 May 2018 14:03:58 +0530 Subject: [PATCH 18/36] more fixes --- core/domain/exp_services.py | 3 +++ core/storage/suggestion/gae_models.py | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/core/domain/exp_services.py b/core/domain/exp_services.py index 6eb1e7638efa..2ed9d670a232 100644 --- a/core/domain/exp_services.py +++ b/core/domain/exp_services.py @@ -2138,6 +2138,9 @@ def find_all_values_for_key(key, dictionary): Returns: list. The values of the key in the given dictionary. + + Yields: + str. The value of the given key. """ if isinstance(dictionary, list): for d in dictionary: diff --git a/core/storage/suggestion/gae_models.py b/core/storage/suggestion/gae_models.py index 7e494f0e2d43..71db1942c97f 100644 --- a/core/storage/suggestion/gae_models.py +++ b/core/storage/suggestion/gae_models.py @@ -203,7 +203,7 @@ def get_suggestions_reviewed_by(cls, final_reviewer_id): """Gets all suggestions that have been reviewed by the given user. Args: - reviewer_id: str. The ID of the reviewer of the suggestion. + final_reviewer_id: str. The ID of the reviewer of the suggestion. Returns: list(SuggestionModel). A list of suggestions reviewed by the given From 7bfcffa9f3686b5674a31379fae4629c0daf83f6 Mon Sep 17 00:00:00 2001 From: Apurv-Bajaj Date: Wed, 23 May 2018 14:06:28 +0530 Subject: [PATCH 19/36] some more fixes --- core/storage/suggestion/gae_models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/storage/suggestion/gae_models.py b/core/storage/suggestion/gae_models.py index 71db1942c97f..23784df8aced 100644 --- a/core/storage/suggestion/gae_models.py +++ b/core/storage/suggestion/gae_models.py @@ -120,7 +120,7 @@ def create( Args: suggestion_type: str. The type of the suggestion. target_type: str. The type of target entity being edited. - target_id: str, The ID of the target entity being edited. + target_id: str. The ID of the target entity being edited. target_version_at_submission: int. The version number of the target entity at the time of creation of the suggestion. status: str. The status of the suggestion. From bd9548b88017e5352772915aa05f2dcc8a1cc0a1 Mon Sep 17 00:00:00 2001 From: Apurv-Bajaj Date: Wed, 23 May 2018 15:24:29 +0530 Subject: [PATCH 20/36] add tests --- scripts/custom_lint_checks_test.py | 101 ++++++++++++++++++----------- 1 file changed, 64 insertions(+), 37 deletions(-) diff --git a/scripts/custom_lint_checks_test.py b/scripts/custom_lint_checks_test.py index fd6571ccc27d..6617a2722db6 100644 --- a/scripts/custom_lint_checks_test.py +++ b/scripts/custom_lint_checks_test.py @@ -80,48 +80,75 @@ def test(test_var_one, test_var_two=4, test_var_three=5, test_var_four="test_che func_call_node_three) -class HangingIndentCheckerTest(unittest.TestCase): + class HangingIndentCheckerTest(unittest.TestCase): - def test_finds_hanging_indent(self): + def test_finds_hanging_indent(self): + checker_test_object = testutils.CheckerTestCase() + checker_test_object.CHECKER_CLASS = ( + custom_lint_checks.HangingIndentChecker) + checker_test_object.setup_method() + node1 = astroid.scoped_nodes.Module(name='test', doc='Custom test') + temp_file = tempfile.NamedTemporaryFile() + filename = temp_file.name + with open(filename, 'w') as tmp: + tmp.write( + """self.post_json('/ml/trainedclassifierhandler', self.payload, + expect_errors=True, expected_status_int=401) + """) + node1.file = filename + node1.path = filename + + checker_test_object.checker.process_module(node1) + + with checker_test_object.assertAddsMessages( + testutils.Message( + msg_id='no-break-after-hanging-indent', + line=1 + ), + ): + temp_file.close() + + node2 = astroid.scoped_nodes.Module(name='test', doc='Custom test') + + temp_file = tempfile.NamedTemporaryFile() + filename = temp_file.name + with open(filename, 'w') as tmp: + tmp.write( + """master_translation_dict = json.loads( + utils.get_file_contents(os.path.join( + os.getcwd(), 'assets', 'i18n', 'en.json'))) + """) + node2.file = filename + node2.path = filename + + checker_test_object.checker.process_module(node2) + + with checker_test_object.assertNoMessages(): + temp_file.close() + + +class DocstringParameterCheckerTest(unittest.TestCase): + + def test_finds_docstring_parameter(self): checker_test_object = testutils.CheckerTestCase() checker_test_object.CHECKER_CLASS = ( - custom_lint_checks.HangingIndentChecker) + custom_lint_checks.DocstringParameterChecker) checker_test_object.setup_method() - node1 = astroid.scoped_nodes.Module(name='test', doc='Custom test') - temp_file = tempfile.NamedTemporaryFile() - filename = temp_file.name - with open(filename, 'w') as tmp: - tmp.write( - """self.post_json('/ml/trainedclassifierhandler', self.payload, - expect_errors=True, expected_status_int=401) - """) - node1.file = filename - node1.path = filename - - checker_test_object.checker.process_module(node1) - - with checker_test_object.assertAddsMessages( - testutils.Message( - msg_id='no-break-after-hanging-indent', - line=1 - ), - ): - temp_file.close() - - node2 = astroid.scoped_nodes.Module(name='test', doc='Custom test') + func_node = astroid.extract_node(""" + def test(test_var_one, test_var_two, test_var_three): #@ + \"\"\"Function to test docstring parameters. - temp_file = tempfile.NamedTemporaryFile() - filename = temp_file.name - with open(filename, 'w') as tmp: - tmp.write( - """master_translation_dict = json.loads( - utils.get_file_contents(os.path.join( - os.getcwd(), 'assets', 'i18n', 'en.json'))) - """) - node2.file = filename - node2.path = filename + Args: + test_var_one: int. First test variable. + test_var_two: str. Second test variable. + test_var_three: list(str). Third test variable. - checker_test_object.checker.process_module(node2) + Returns: + str. The test result. + Yields: + list(str). The test list. + \"\"\" + """) with checker_test_object.assertNoMessages(): - temp_file.close() + checker_test_object.checker.visit_functiondef(func_node) From c83ea68b4a80695ec3f044f7870c7f5f2bd03324 Mon Sep 17 00:00:00 2001 From: Apurv-Bajaj Date: Wed, 23 May 2018 15:34:50 +0530 Subject: [PATCH 21/36] lint fix --- scripts/custom_lint_checks_test.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/scripts/custom_lint_checks_test.py b/scripts/custom_lint_checks_test.py index 6617a2722db6..3828126da3a2 100644 --- a/scripts/custom_lint_checks_test.py +++ b/scripts/custom_lint_checks_test.py @@ -92,8 +92,8 @@ def test_finds_hanging_indent(self): filename = temp_file.name with open(filename, 'w') as tmp: tmp.write( - """self.post_json('/ml/trainedclassifierhandler', self.payload, - expect_errors=True, expected_status_int=401) + """self.post_json('/ml/trainedclassifierhandler', + self.payload, expect_errors=True, expected_status_int=401) """) node1.file = filename node1.path = filename @@ -128,7 +128,7 @@ def test_finds_hanging_indent(self): class DocstringParameterCheckerTest(unittest.TestCase): - + def test_finds_docstring_parameter(self): checker_test_object = testutils.CheckerTestCase() checker_test_object.CHECKER_CLASS = ( From b6f14f1f4f08ded1ea4c8c380b8ed24d163c5752 Mon Sep 17 00:00:00 2001 From: Apurv-Bajaj Date: Wed, 23 May 2018 16:18:44 +0530 Subject: [PATCH 22/36] Update returns --- core/storage/feedback/gae_models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/storage/feedback/gae_models.py b/core/storage/feedback/gae_models.py index 04a1c81b98fc..9dd1351c9088 100644 --- a/core/storage/feedback/gae_models.py +++ b/core/storage/feedback/gae_models.py @@ -90,7 +90,7 @@ def put(self, update_last_updated_time=True): last_updated_field of the thread. Returns: - list. + thread. The thread entity. """ if update_last_updated_time: self.last_updated = datetime.datetime.utcnow() From 308b750d5dfdc3b01b989483695ae5032b7eb6e9 Mon Sep 17 00:00:00 2001 From: Apurv-Bajaj Date: Thu, 24 May 2018 11:15:33 +0530 Subject: [PATCH 23/36] Review changes(1) --- core/domain/config_domain.py | 2 +- core/domain/exp_domain.py | 4 +- core/domain/exp_services.py | 3 +- core/domain/question_services.py | 4 +- core/storage/exploration/gae_models.py | 4 +- core/storage/feedback/gae_models.py | 2 +- scripts/_check_docs_utils.py | 67 +++++++++++------------- scripts/custom_lint_checks.py | 72 +++++++++++--------------- 8 files changed, 70 insertions(+), 88 deletions(-) diff --git a/core/domain/config_domain.py b/core/domain/config_domain.py index 73c9e4ea1548..3aa38c725fec 100644 --- a/core/domain/config_domain.py +++ b/core/domain/config_domain.py @@ -208,7 +208,7 @@ def init_config_property(cls, name, instance): Args: name: str. The name of the configuration property. - instance: instance. The instance of the configuration property. + instance: Registry. The instance of the configuration property. """ cls._config_registry[name] = instance diff --git a/core/domain/exp_domain.py b/core/domain/exp_domain.py index 5c21790c977f..7612055477ed 100644 --- a/core/domain/exp_domain.py +++ b/core/domain/exp_domain.py @@ -576,7 +576,7 @@ def to_html(self, params): Returns: str. The HTML string that results after stripping - out unrecognized tags and attributes. + out unrecognized tags and attributes. """ if not isinstance(params, dict): raise Exception( @@ -879,7 +879,7 @@ def validate(self, interaction, exp_param_specs_dict): exp_param_specs_dict: dict. A dict of all parameters used in the exploration. Keys are parameter names and values are ParamSpec value objects with an object type property (obj_type). - interaction: obj. The interaction object. + interaction: InteractionInstance. The interaction object. Raises: ValidationError: One or more attributes of the AnswerGroup are diff --git a/core/domain/exp_services.py b/core/domain/exp_services.py index 2ed9d670a232..0f1e7c6c486b 100644 --- a/core/domain/exp_services.py +++ b/core/domain/exp_services.py @@ -1587,7 +1587,8 @@ def get_next_page_of_all_non_private_commits( urlsafe_start_cursor: str. If this is not None, then the returned commits start from cursor location. Otherwise they start from the beginning of the list of commits. - max_age: datetime.timedelta. + max_age: datetime.timedelta. The maximum age to which all non private + commits are fetch from the ExplorationCommitLogEntry. Returns: tuple. A 3-tuple consisting of: diff --git a/core/domain/question_services.py b/core/domain/question_services.py index c16dc24e081f..c8bb71a186db 100644 --- a/core/domain/question_services.py +++ b/core/domain/question_services.py @@ -37,7 +37,7 @@ def _create_new_question(committer_id, question, commit_message): commit_message: str. A description of changes made to the question. Returns: - model_id. The ID of the model. + str. The ID of the model. """ model = question_models.QuestionModel.create( title=question.title, @@ -62,7 +62,7 @@ def add_question(committer_id, question): question: Question. Question to be saved. Returns: - question_id. The ID of the question. + str. The ID of the question. """ question.validate() commit_message = ( diff --git a/core/storage/exploration/gae_models.py b/core/storage/exploration/gae_models.py index 28b84dba82c4..14f3945e28a7 100644 --- a/core/storage/exploration/gae_models.py +++ b/core/storage/exploration/gae_models.py @@ -318,7 +318,7 @@ def _get_instance_id(cls, exp_id, exp_version): Returns: str. A string containing exploration ID and - exploration version. + exploration version. """ return 'exploration-%s-%s' % (exp_id, exp_version) @@ -610,7 +610,7 @@ def _generate_instance_id(cls, exp_id, exp_version): Returns: str. A string containing exploration ID and - exploration version. + exploration version. """ return '%s.%d' % (exp_id, exp_version) diff --git a/core/storage/feedback/gae_models.py b/core/storage/feedback/gae_models.py index 9dd1351c9088..cc3ffe8a16c4 100644 --- a/core/storage/feedback/gae_models.py +++ b/core/storage/feedback/gae_models.py @@ -195,7 +195,7 @@ def get_by_exp_and_thread_id(cls, exploration_id, thread_id): thread_id: str. ID of the thread. Returns: - FeedbackThreadModel. or None if the thread is not found or is + FeedbackThreadModel|None. None if the thread is not found or is already deleted. """ return cls.get_by_id(cls.generate_full_thread_id( diff --git a/scripts/_check_docs_utils.py b/scripts/_check_docs_utils.py index 9a779f2d6cde..b68423e0cf3b 100644 --- a/scripts/_check_docs_utils.py +++ b/scripts/_check_docs_utils.py @@ -1,18 +1,11 @@ # coding: utf-8 -# Copyright 2018 The Oppia Authors. All Rights Reserved. -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS-IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. +# The following code was adapted from PyCQA for use +# in the Oppia project. See +# https://github.com/PyCQA/pylint/blob/master/pylint/extensions/_check_docs_utils.py +# for the source origin. +# Licensed under the GPL: https://www.gnu.org/licenses/old-licenses/gpl-2.0.html +# For details: https://github.com/PyCQA/pylint/blob/master/COPYING """Utility methods for docstring checking.""" @@ -29,10 +22,11 @@ def space_indentation(s): """The number of leading spaces in a string - :param str s: input string + Args: + s: str. The input string. - :rtype: int - :return: number of leading spaces + Returns: + int. The number of leading spaces. """ return len(s) - len(s.lstrip(' ')) @@ -40,12 +34,12 @@ def space_indentation(s): def get_setters_property_name(node): """Get the name of the property that the given node is a setter for. - :param node: The node to get the property name for. - :type node: str + Args: + node: str. The node to get the property name for. - :rtype: str or None - :returns: The name of the property that the node is a setter for, - or None if one could not be found. + Returns: + str|None. The name of the property that the node is a setter for, + or None if one could not be found. """ decorators = node.decorators.nodes if node.decorators else [] for decorator in decorators: @@ -59,12 +53,12 @@ def get_setters_property_name(node): def get_setters_property(node): """Get the property node for the given setter node. - :param node: The node to get the property for. - :type node: astroid.FunctionDef + Args: + node: astroid.FunctionDef. The node to get the property for. - :rtype: astroid.FunctionDef or None - :returns: The node relating to the property of the given setter node, - or None if one could not be found. + Returns: + astroid.FunctionDef|None. The node relating to the property of + the given setter node, or None if one could not be found. """ property_ = None @@ -83,12 +77,12 @@ def get_setters_property(node): def returns_something(return_node): """Check if a return node returns a value other than None. - :param return_node: The return node to check. - :type return_node: astroid.Return + Args: + return_node: astroid.Return. The return node to check. - :rtype: bool - :return: True if the return node returns a value other than None, - False otherwise. + Returns: + bool. True if the return node returns a value + other than None, False otherwise. """ returns = return_node.value @@ -106,11 +100,12 @@ def possible_exc_types(node): Caught exception types are ignored. - :param node: The raise node to find exception types for. - :type node: astroid.node_classes.NodeNG - - :returns: A list of exception types possibly raised by :param:`node`. - :rtype: set(str) + Args: + node: astroid.node_classes.NodeNG. The raise + to find exception types for. + + Returns: + set(str). A list of exception types. """ excs = [] if isinstance(node.exc, astroid.Name): diff --git a/scripts/custom_lint_checks.py b/scripts/custom_lint_checks.py index 42f35d67fece..cc9070e2f219 100644 --- a/scripts/custom_lint_checks.py +++ b/scripts/custom_lint_checks.py @@ -149,6 +149,8 @@ def process_module(self, node): 'no-break-after-hanging-indent', line=line_num + 1) +# This class has been derived from +# https://github.com/PyCQA/pylint/blob/master/pylint/extensions/docparams.py#L26. class DocstringParameterChecker(BaseChecker): """Checker for Sphinx, Google, or Numpy style docstrings @@ -163,14 +165,8 @@ class DocstringParameterChecker(BaseChecker): * Check that all explicitly raised exceptions in a function are documented in the function docstring. Caught exceptions are ignored. - Activate this checker by adding the line:: - - load-plugins=pylint.extensions.docparams - - to the ``MASTER`` section of your ``.pylintrc``. - - :param linter: linter object - :type linter: :class:`pylint.lint.PyLinter` + Args: + linter: Pylinter. The linter object. """ __implements__ = IAstroidChecker @@ -263,8 +259,9 @@ class DocstringParameterChecker(BaseChecker): def visit_functiondef(self, node): """Called for function and method definitions (def). - :param node: Node for a function or method definition in the AST - :type node: :class:`astroid.scoped_nodes.Function` + Args: + node: astroid.scoped_nodes.Function. Node for a function or + method definition in the AST. """ node_doc = utils.docstringify(node.doc) self.check_functiondef_params(node, node_doc) @@ -428,20 +425,15 @@ def check_arguments_in_docstring( checker assumes that the parameters are documented in another format and the absence is tolerated. - :param doc: Docstring for the function, method or class. - :type doc: str - - :param arguments_node: Arguments node for the function, method or - class constructor. - :type arguments_node: :class:`astroid.scoped_nodes.Arguments` - - :param warning_node: The node to assign the warnings to - :type warning_node: :class:`astroid.scoped_nodes.Node` - - :param accept_no_param_doc: Whether or not to allow no parameters - to be documented. - If None then this value is read from the configuration. - :type accept_no_param_doc: bool or None + Args: + doc: str. Docstring for the function, method or class. + arguments_node: astroid.scoped_nodes.Arguments. Arguments node + for the function, method or class constructor. + warning_node: astroid.scoped_nodes.Node. The node to assign + the warnings to. + accept_no_param_doc: bool|None. Whether or not to allow + no parameters to be documented. If None then + this value is read from the configuration. """ # Tolerate missing param or type declarations if there is a link to # another method carrying the same name. @@ -478,13 +470,11 @@ def _compare_missing_args( """Compare the found argument names with the expected ones and generate a message if there are arguments missing. - :param set found_argument_names: argument names found in the - docstring - - :param str message_id: pylint message id - - :param not_needed_names: names that may be omitted - :type not_needed_names: set of str + Args: + found_argument_names: set. Argument names found in the + docstring. + message_id: str. Pylint message id. + not_needed_names: set(str). Names that may be omitted. """ if not tolerate_missing_params: missing_argument_names = ( @@ -502,13 +492,11 @@ def _compare_different_args( """Compare the found argument names with the expected ones and generate a message if there are extra arguments found. - :param set found_argument_names: argument names found in the - docstring - - :param str message_id: pylint message id - - :param not_needed_names: names that may be omitted - :type not_needed_names: set of str + Args: + found_argument_names: set. Argument names found in the + docstring. + message_id: str. Pylint message id. + not_needed_names: set(str). Names that may be omitted. """ differing_argument_names = ( (expected_argument_names ^ found_argument_names) @@ -547,11 +535,9 @@ def _handle_no_raise_doc(self, excs, node): def _add_raise_message(self, missing_excs, node): """Adds a message on :param:`node` for the missing exception type. - :param missing_excs: A list of missing exception types. - :type missing_excs: list - - :param node: The node show the message on. - :type node: astroid.node_classes.NodeNG + Args: + missing_excs: list. A list of missing exception types. + node: astroid.node_classes.NodeNG. The node show the message on. """ if not missing_excs: return From 2110909d958738ab0c48403958b16610f833dc18 Mon Sep 17 00:00:00 2001 From: Apurv-Bajaj Date: Thu, 24 May 2018 11:18:31 +0530 Subject: [PATCH 24/36] lint fix --- scripts/_check_docs_utils.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/_check_docs_utils.py b/scripts/_check_docs_utils.py index b68423e0cf3b..995e6d4be766 100644 --- a/scripts/_check_docs_utils.py +++ b/scripts/_check_docs_utils.py @@ -2,7 +2,7 @@ # The following code was adapted from PyCQA for use # in the Oppia project. See -# https://github.com/PyCQA/pylint/blob/master/pylint/extensions/_check_docs_utils.py +# https://github.com/PyCQA/pylint/blob/master/pylint/extensions/_check_docs_utils.py # for the source origin. # Licensed under the GPL: https://www.gnu.org/licenses/old-licenses/gpl-2.0.html # For details: https://github.com/PyCQA/pylint/blob/master/COPYING @@ -103,7 +103,7 @@ def possible_exc_types(node): Args: node: astroid.node_classes.NodeNG. The raise to find exception types for. - + Returns: set(str). A list of exception types. """ From 5ceec7169ee74c599de177726f603c9b1df77b8d Mon Sep 17 00:00:00 2001 From: Apurv-Bajaj Date: Thu, 24 May 2018 12:03:06 +0530 Subject: [PATCH 25/36] minor fix --- scripts/_check_docs_utils.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/scripts/_check_docs_utils.py b/scripts/_check_docs_utils.py index 995e6d4be766..c74b9eb8cc06 100644 --- a/scripts/_check_docs_utils.py +++ b/scripts/_check_docs_utils.py @@ -94,10 +94,7 @@ def returns_something(return_node): def possible_exc_types(node): """Gets all of the possible raised exception types for the given raise node. - - .. note:: - - Caught exception types are ignored. + Caught exception types are ignored. Args: From 9075e29851ea895fb6de3de3a9d2e6904502856b Mon Sep 17 00:00:00 2001 From: Apurv-Bajaj Date: Thu, 24 May 2018 17:58:42 +0530 Subject: [PATCH 26/36] Address all review comments except returns --- .gitignore | 5 ++ core/controllers/base.py | 4 +- core/domain/acl_decorators.py | 64 ++++++------- core/domain/collection_services.py | 4 +- core/domain/config_domain.py | 2 +- core/domain/feedback_jobs_continuous.py | 6 +- core/domain/search_services.py | 5 +- core/domain/topic_services.py | 3 +- core/domain/user_jobs_continuous.py | 9 +- core/jobs.py | 10 +-- .../transactions/gae_transaction_services.py | 4 +- scripts/custom_lint_checks.py | 10 ++- scripts/custom_lint_checks_test.py | 90 +++++++++---------- .../_check_docs_utils.py | 0 utils.py | 2 +- 15 files changed, 118 insertions(+), 100 deletions(-) rename {scripts => third_party/python-custom-checks}/_check_docs_utils.py (100%) diff --git a/.gitignore b/.gitignore index f396a379dc90..abd564cb8d96 100644 --- a/.gitignore +++ b/.gitignore @@ -21,3 +21,8 @@ venv/ # directories for dev and prod. Resource directories for prod are generated # via build.py and should not be committed to the repo. build/* + +# The pre_commit_linter.py script uses _check_docs_utils.py placed +# in python-custom-checks directory under third_party directory. +# Therefore it is not ignored. +!third_party/python-custom-checks diff --git a/core/controllers/base.py b/core/controllers/base.py index f5a867088395..66ff7bbcabe1 100755 --- a/core/controllers/base.py +++ b/core/controllers/base.py @@ -443,7 +443,7 @@ def handle_exception(self, exception, unused_debug_mode): """Overwrites the default exception handler. Args: - exception: exception. The exception that was thrown. + exception: Exception. The exception that was thrown. unused_debug_mode: bool. True if the web application is running in debug mode. """ @@ -577,7 +577,7 @@ def is_csrf_token_valid(cls, user_id, token): token: str. The CSRF token to validate. Returns: - bool. + bool. Whether the given CSRF token is valid. """ try: parts = token.split('/') diff --git a/core/domain/acl_decorators.py b/core/domain/acl_decorators.py index a427383cfeb2..c931f5ae9c0d 100644 --- a/core/domain/acl_decorators.py +++ b/core/domain/acl_decorators.py @@ -45,10 +45,10 @@ def test_can_play(self, exploration_id, **kwargs): Args: exploration_id: str. The exploration id. - kwargs: keyword_arguments. + **kwargs: *. Keyword arguments. Returns: - bool. + bool. Whether the user can play the given exploration. """ if exploration_id in feconf.DISABLED_EXPLORATION_IDS: raise self.PageNotFoundException @@ -74,10 +74,10 @@ def test_can_play(self, collection_id, **kwargs): Args: collection_id: str. The collection id. - kwargs: keyword_arguments. + **kwargs: *. Keyword arguments. Returns: - bool. + bool. Whether the user can play the given collection. """ collection_rights = rights_manager.get_collection_rights( collection_id, strict=False) @@ -102,10 +102,10 @@ def test_can_download(self, exploration_id, **kwargs): Args: exploration_id: str. The exploration id. - kwargs: keyword_arguments. + **kwargs: *. Keyword arguments. Returns: - bool. + bool. Whether the user can download the given exploration. """ if exploration_id in feconf.DISABLED_EXPLORATION_IDS: raise base.UserFacingExceptions.PageNotFoundException @@ -132,10 +132,10 @@ def test_can_view_stats(self, exploration_id, **kwargs): Args: exploration_id: str. The exploration id. - kwargs: keyword_arguments. + **kwargs: *. Keyword arguments. Returns: - bool. + bool. Whether the user can view the exploration stats. """ if exploration_id in feconf.DISABLED_EXPLORATION_IDS: raise base.UserFacingExceptions.PageNotFoundException @@ -160,10 +160,10 @@ def test_can_edit(self, collection_id, **kwargs): Args: collection_id: str. The collection id. - kwargs: keyword_arguments. + **kwargs: *. Keyword arguments. Returns: - bool. + bool. Whether the user can edit the collection. """ if not self.user_id: raise base.UserFacingExceptions.NotLoggedInException @@ -353,10 +353,10 @@ def test_can_access(self, exploration_id, **kwargs): Args: exploration_id: str. The exploration id. - kwargs: keyword_arguments. + **kwargs: *. Keyword arguments. Returns: - bool. + bool. Whether the user can view the exploration feedback. """ if not self.user_id: raise base.UserFacingExceptions.NotLoggedInException @@ -388,10 +388,10 @@ def test_can_rate(self, exploration_id, **kwargs): Args: exploration_id: str. The exploration id. - kwargs: keyword_arguments. + **kwargs: *. Keyword arguments. Returns: - bool. + bool. Whether the user can rate the exploration. """ if (role_services.ACTION_RATE_ANY_PUBLIC_EXPLORATION in self.user.actions): @@ -412,10 +412,10 @@ def test_can_flag(self, exploration_id, **kwargs): Args: exploration_id: str. The exploration id. - kwargs: keyword_arguments. + **kwargs: *. Keyword arguments. Returns: - bool. + bool. Whether the user can flag the exploration. """ if role_services.ACTION_FLAG_EXPLORATION in self.user.actions: return handler(self, exploration_id, **kwargs) @@ -450,10 +450,10 @@ def test_can_edit(self, exploration_id, **kwargs): Args: exploration_id: str. The exploration id. - kwargs: keyword_arguments. + **kwargs: *. Keyword arguments. Returns: - bool. + bool. Whether the user can edit the exploration. """ if not self.user_id: raise base.UserFacingExceptions.NotLoggedInException @@ -482,10 +482,10 @@ def test_can_delete(self, exploration_id, **kwargs): Args: exploration_id: str. The exploration id. - kwargs: keyword_arguments. + **kwargs: *. Keyword arguments. Returns: - bool. + bool. Whether the user can delete the exploration. """ if not self.user_id: raise base.UserFacingExceptions.NotLoggedInException @@ -515,10 +515,11 @@ def test_can_suggest(self, exploration_id, **kwargs): Args: exploration_id: str. The exploration id. - kwargs: keyword_arguments. + **kwargs: *. Keyword arguments. Returns: - bool. + bool. Whether the user can make suggestions to an + exploration. """ if (role_services.ACTION_SUGGEST_CHANGES_TO_EXPLORATION in self.user.actions): @@ -540,11 +541,11 @@ def test_can_publish(self, exploration_id, *args, **kwargs): Args: exploration_id: str. The exploration id. - args: arguments. - kwargs: keyword_arguments. + *args: arguments. + **kwargs: *. Keyword arguments. Returns: - bool. + bool. Whether the user can publish the exploration. """ exploration_rights = rights_manager.get_exploration_rights( exploration_id, strict=False) @@ -571,10 +572,10 @@ def test_can_publish_collection(self, collection_id, **kwargs): Args: collection_id: str. The collection id. - kwargs: keyword_arguments. + **kwargs: *. Keyword arguments. Returns: - bool. + bool. Whether the user can publish the collection. """ collection_rights = rights_manager.get_collection_rights( collection_id) @@ -600,10 +601,10 @@ def test_can_unpublish_collection(self, collection_id, **kwargs): Args: collection_id: str. The collection id. - kwargs: keyword_arguments. + **kwargs: *. Keyword arguments. Returns: - bool. + bool. Whether the user can unpublish the collection. """ collection_rights = rights_manager.get_collection_rights( collection_id) @@ -631,10 +632,11 @@ def test_can_modify(self, exploration_id, **kwargs): Args: exploration_id: str. The exploration id. - kwargs: keyword_arguments. + **kwargs: *. Keyword arguments. Returns: - bool. + bool. Whether the user can modify the rights related to + an exploration. """ exploration_rights = rights_manager.get_exploration_rights( exploration_id, strict=False) diff --git a/core/domain/collection_services.py b/core/domain/collection_services.py index 917e83f0e221..7f79e8da14d5 100644 --- a/core/domain/collection_services.py +++ b/core/domain/collection_services.py @@ -953,7 +953,7 @@ def compute_summary_of_collection(collection, contributor_id_to_add): object and return it. Args: - collection: Collection. The collection object. + collection: Collection. The domain object. contributor_id_to_add: str. ID of the contributor to be added to the collection summary. @@ -1094,7 +1094,7 @@ def save_new_collection_from_yaml(committer_id, yaml_content, collection_id): collection_id: str. ID of the saved collection. Returns: - collection. The collection object. + Collection. The domain object. """ collection = collection_domain.Collection.from_yaml( collection_id, yaml_content) diff --git a/core/domain/config_domain.py b/core/domain/config_domain.py index 3aa38c725fec..d5024ac9bff8 100644 --- a/core/domain/config_domain.py +++ b/core/domain/config_domain.py @@ -208,7 +208,7 @@ def init_config_property(cls, name, instance): Args: name: str. The name of the configuration property. - instance: Registry. The instance of the configuration property. + instance: *. The instance of the configuration property. """ cls._config_registry[name] = instance diff --git a/core/domain/feedback_jobs_continuous.py b/core/domain/feedback_jobs_continuous.py index 93dc0f4c8b81..8e8227b175ac 100644 --- a/core/domain/feedback_jobs_continuous.py +++ b/core/domain/feedback_jobs_continuous.py @@ -63,9 +63,9 @@ def _handle_incoming_event(cls, active_realtime_layer, event_type, *args): active_realtime_layer: int. The currently active realtime datastore layer. event_type: str. The event triggered by the student. - args: variable_length_argument_list. The first element of *args - corresponds to the id of the exploration currently being - played. + *args: list(*). Variable length argument list. The + first element of *args corresponds to the id + of the exploration currently being played. """ exp_id = args[0] diff --git a/core/domain/search_services.py b/core/domain/search_services.py index 9dc5ec5b3c10..f0a4057e5086 100644 --- a/core/domain/search_services.py +++ b/core/domain/search_services.py @@ -77,7 +77,8 @@ def _should_index_exploration(exp_summary): exp_summary: ExpSummaryModel. ExplorationSummary domain object. Returns: - bool. + bool. Whether the given exploration should be indexed for future + search queries. """ rights = rights_manager.get_exploration_rights(exp_summary.id) return rights.status != rights_manager.ACTIVITY_STATUS_PRIVATE @@ -167,7 +168,7 @@ def _should_index_collection(collection): collection: CollectionSummaryModel. Returns: - bool. + bool. Whether a particular collection should be indexed. """ rights = rights_manager.get_collection_rights(collection.id) return rights.status != rights_manager.ACTIVITY_STATUS_PRIVATE diff --git a/core/domain/topic_services.py b/core/domain/topic_services.py index 98ac3db7370c..44c37e1ecc11 100644 --- a/core/domain/topic_services.py +++ b/core/domain/topic_services.py @@ -84,7 +84,8 @@ def get_topic_rights(topic_id, strict=True): Args: topic_id: str. ID of the topic. - strict: bool. + strict: bool. Whether to fail noisily if no topic with a given id + exists in the datastore. Returns: TopicRights. The rights object associated with the given topic. diff --git a/core/domain/user_jobs_continuous.py b/core/domain/user_jobs_continuous.py index a632a60b9c5b..d77e141f5023 100644 --- a/core/domain/user_jobs_continuous.py +++ b/core/domain/user_jobs_continuous.py @@ -124,8 +124,9 @@ def _get_most_recent_activity_commits( as those from the Oppia migration bot). Args: - activity_model_cls: storage_layer_object. The storage layer - object for an activity, such as exp_models.ExplorationModel. + activity_model_cls: ExplorationModel|CollectionModel. The + storage layer object for an activity, such as + exp_models.ExplorationModel. activity_ids_list: list(str). A list of activity IDs (such as exploration IDS) for which the latest commits will be retrieved. @@ -376,7 +377,7 @@ def _handle_incoming_event(cls, active_realtime_layer, event_type, *args): triggered and the total play count is incremented. If he/she rates an exploration, an event of type `rate` is triggered and average rating of the realtime model is refreshed. - args: arguments. + *args: list(*). If event_type is 'start', then this is a 1-element list with following entry: - str. The ID of the exploration currently being played. @@ -451,7 +452,7 @@ def get_dashboard_stats(cls, user_id): user_id: str. The ID of the user. Returns: - dict. with the following keys: + dict. This has the following keys: 'total_plays': int. Number of times the user's explorations were played. 'num_ratings': int. Number of times the explorations have been diff --git a/core/jobs.py b/core/jobs.py index 7a3ee2e9b937..41a689d0c79a 100644 --- a/core/jobs.py +++ b/core/jobs.py @@ -948,7 +948,7 @@ def validate(cls, unused_mapper_spec): BadReaderParamsError: Required parameters are missing or invalid. Returns: - bool. True. + bool. Whether mapper spec and all mapper patterns are valid. """ return True # TODO. @@ -1217,8 +1217,8 @@ def _handle_incoming_event( a student starts an exploration, event of type `start` is triggered. If he/she completes an exploration, event of type `complete` is triggered. - args: list(*). Forwarded to the _handle_event() method. - kwargs: dict(* : *). Forwarded to the _handle_event() method. + *args: list(*). Forwarded to the _handle_event() method. + **kwargs: dict(* : *). Forwarded to the _handle_event() method. """ raise NotImplementedError( 'Subclasses of BaseContinuousComputationManager must implement ' @@ -1446,8 +1446,8 @@ def on_incoming_event(cls, event_type, *args, **kwargs): a student starts an exploration, event of type `start` is triggered. If he/she completes an exploration, event of type `complete` is triggered. - args: arguments. Forwarded to _handle_event() method. - kwargs: keyword_arguments. Forwarded to _handle_event() method. + *args: list(*). Forwarded to _handle_event() method. + **kwargs: *. Forwarded to _handle_event() method. """ realtime_layers = [0, 1] for layer in realtime_layers: diff --git a/core/platform/transactions/gae_transaction_services.py b/core/platform/transactions/gae_transaction_services.py index e5bcfdb22ba3..7010ae77d94b 100644 --- a/core/platform/transactions/gae_transaction_services.py +++ b/core/platform/transactions/gae_transaction_services.py @@ -53,8 +53,8 @@ def toplevel_wrapper(*args, **kwargs): https://developers.google.com/appengine/docs/python/ndb/async#intro Args: - args: variable_length_argument_list. - kwargs: arbitrary_keyword_arguments. + *args: list(*). Variable length argument list. + **kwargs: *. Arbitrary keyword arguments. Returns: app. The entire app toplevel. diff --git a/scripts/custom_lint_checks.py b/scripts/custom_lint_checks.py index cc9070e2f219..85834e2577c3 100644 --- a/scripts/custom_lint_checks.py +++ b/scripts/custom_lint_checks.py @@ -18,7 +18,8 @@ from __future__ import division from __future__ import print_function -import _check_docs_utils as utils +import os +import sys import astroid from pylint.checkers import BaseChecker @@ -26,6 +27,13 @@ from pylint.interfaces import IAstroidChecker from pylint.interfaces import IRawChecker +path = os.path.join('third_party', 'python-custom-checks') +sys.path.insert(0, path) + +# pylint: disable=wrong-import-position +import _check_docs_utils as utils # isort:skip +# pylint: enable=wrong-import-position + class ExplicitKwargsChecker(BaseChecker): """Custom pylint checker which checks for explicit keyword arguments diff --git a/scripts/custom_lint_checks_test.py b/scripts/custom_lint_checks_test.py index 3828126da3a2..4610385b3759 100644 --- a/scripts/custom_lint_checks_test.py +++ b/scripts/custom_lint_checks_test.py @@ -80,51 +80,51 @@ def test(test_var_one, test_var_two=4, test_var_three=5, test_var_four="test_che func_call_node_three) - class HangingIndentCheckerTest(unittest.TestCase): - - def test_finds_hanging_indent(self): - checker_test_object = testutils.CheckerTestCase() - checker_test_object.CHECKER_CLASS = ( - custom_lint_checks.HangingIndentChecker) - checker_test_object.setup_method() - node1 = astroid.scoped_nodes.Module(name='test', doc='Custom test') - temp_file = tempfile.NamedTemporaryFile() - filename = temp_file.name - with open(filename, 'w') as tmp: - tmp.write( - """self.post_json('/ml/trainedclassifierhandler', - self.payload, expect_errors=True, expected_status_int=401) - """) - node1.file = filename - node1.path = filename - - checker_test_object.checker.process_module(node1) - - with checker_test_object.assertAddsMessages( - testutils.Message( - msg_id='no-break-after-hanging-indent', - line=1 - ), - ): - temp_file.close() - - node2 = astroid.scoped_nodes.Module(name='test', doc='Custom test') - - temp_file = tempfile.NamedTemporaryFile() - filename = temp_file.name - with open(filename, 'w') as tmp: - tmp.write( - """master_translation_dict = json.loads( - utils.get_file_contents(os.path.join( - os.getcwd(), 'assets', 'i18n', 'en.json'))) - """) - node2.file = filename - node2.path = filename - - checker_test_object.checker.process_module(node2) - - with checker_test_object.assertNoMessages(): - temp_file.close() +class HangingIndentCheckerTest(unittest.TestCase): + + def test_finds_hanging_indent(self): + checker_test_object = testutils.CheckerTestCase() + checker_test_object.CHECKER_CLASS = ( + custom_lint_checks.HangingIndentChecker) + checker_test_object.setup_method() + node1 = astroid.scoped_nodes.Module(name='test', doc='Custom test') + temp_file = tempfile.NamedTemporaryFile() + filename = temp_file.name + with open(filename, 'w') as tmp: + tmp.write( + """self.post_json('/ml/trainedclassifierhandler', + self.payload, expect_errors=True, expected_status_int=401) + """) + node1.file = filename + node1.path = filename + + checker_test_object.checker.process_module(node1) + + with checker_test_object.assertAddsMessages( + testutils.Message( + msg_id='no-break-after-hanging-indent', + line=1 + ), + ): + temp_file.close() + + node2 = astroid.scoped_nodes.Module(name='test', doc='Custom test') + + temp_file = tempfile.NamedTemporaryFile() + filename = temp_file.name + with open(filename, 'w') as tmp: + tmp.write( + """master_translation_dict = json.loads( + utils.get_file_contents(os.path.join( + os.getcwd(), 'assets', 'i18n', 'en.json'))) + """) + node2.file = filename + node2.path = filename + + checker_test_object.checker.process_module(node2) + + with checker_test_object.assertNoMessages(): + temp_file.close() class DocstringParameterCheckerTest(unittest.TestCase): diff --git a/scripts/_check_docs_utils.py b/third_party/python-custom-checks/_check_docs_utils.py similarity index 100% rename from scripts/_check_docs_utils.py rename to third_party/python-custom-checks/_check_docs_utils.py diff --git a/utils.py b/utils.py index afd7abf5aa2e..7512927132d2 100644 --- a/utils.py +++ b/utils.py @@ -633,7 +633,7 @@ def _get_short_language_description(full_language_description): full_language_description: str. Short description of the language. Returns: - full_language_description: str. Short description of the language. + str. Short description of the language. """ if ' (' not in full_language_description: return full_language_description From 9e66a21b8331903266897889c49283eac8dcc284 Mon Sep 17 00:00:00 2001 From: Apurv-Bajaj Date: Thu, 24 May 2018 18:01:10 +0530 Subject: [PATCH 27/36] lint fix --- scripts/custom_lint_checks.py | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts/custom_lint_checks.py b/scripts/custom_lint_checks.py index 85834e2577c3..0f0a9f41bb0e 100644 --- a/scripts/custom_lint_checks.py +++ b/scripts/custom_lint_checks.py @@ -20,6 +20,7 @@ import os import sys + import astroid from pylint.checkers import BaseChecker From c616ecd6881462b0bc0d577c34a83c88471ecebb Mon Sep 17 00:00:00 2001 From: Apurv-Bajaj Date: Thu, 24 May 2018 18:43:00 +0530 Subject: [PATCH 28/36] fix yields --- core/domain/stats_jobs_continuous.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/core/domain/stats_jobs_continuous.py b/core/domain/stats_jobs_continuous.py index b0bcafb3d084..6004dc7dca83 100644 --- a/core/domain/stats_jobs_continuous.py +++ b/core/domain/stats_jobs_continuous.py @@ -106,6 +106,18 @@ def reduce(key, stringified_values): Yields: Error. + - Expected a single version when aggregating answers for: + Occurs when the versions list contains multiple versions + instead of a specific version. + - Expected exactly one interaction ID for exploration: + Occurs when there is not exactly one interaction ID + for each exploration and version. + - Expected at least one item ID for exploration: + Occurs when there is not atleast one Item ID for + each exploration and version. + - Ignoring answers submitted to version: + Occurs when version mismatches and the new + version has a different interaction ID. """ exploration_id, exploration_version, state_name = key.split(':') From aa63883131afe7f58cbf74c5f02497c3e5636987 Mon Sep 17 00:00:00 2001 From: Apurv-Bajaj Date: Thu, 24 May 2018 20:24:32 +0530 Subject: [PATCH 29/36] remove unnecessary returns --- core/controllers/base.py | 5 +---- core/domain/email_manager.py | 15 +++------------ 2 files changed, 4 insertions(+), 16 deletions(-) diff --git a/core/controllers/base.py b/core/controllers/base.py index 66ff7bbcabe1..da3431a6855b 100755 --- a/core/controllers/base.py +++ b/core/controllers/base.py @@ -211,9 +211,6 @@ def dispatch(self): Raises: Exception: The CSRF token is missing. UnauthorizedUserException: The CSRF token is invalid. - - Returns: - None """ # If the request is to the old demo server, redirect it permanently to # the new demo server. @@ -247,7 +244,7 @@ def dispatch(self): '%s: payload %s', e, self.payload) - return self.handle_exception(e, self.app.debug) + self.handle_exception(e, self.app.debug) super(BaseHandler, self).dispatch() diff --git a/core/domain/email_manager.py b/core/domain/email_manager.py index c3598e8854ff..59761446824d 100644 --- a/core/domain/email_manager.py +++ b/core/domain/email_manager.py @@ -224,9 +224,6 @@ def _send_email( the email. reply_to_id: str or None. The unique reply-to id used in reply-to email address sent to recipient. - - Returns: - None. """ if sender_name is None: @@ -266,7 +263,7 @@ def _send_email_in_transaction(): recipient_id, recipient_email, sender_id, sender_name_email, intent, email_subject, cleaned_html_body, datetime.datetime.utcnow()) - return transaction_services.run_in_transaction(_send_email_in_transaction) + transaction_services.run_in_transaction(_send_email_in_transaction) def _send_bulk_mail( @@ -284,9 +281,6 @@ def _send_bulk_mail( sender_name: str. The name to be shown in the "sender" field of the email. instance_id: str or None. The ID of the BulkEmailModel entity instance. - - Returns: - None. """ _require_sender_id_is_valid(intent, sender_id) @@ -318,7 +312,7 @@ def _send_bulk_mail_in_transaction(instance_id=None): instance_id, recipient_ids, sender_id, sender_name_email, intent, email_subject, cleaned_html_body, datetime.datetime.utcnow()) - return transaction_services.run_in_transaction( + transaction_services.run_in_transaction( _send_bulk_mail_in_transaction, instance_id) @@ -934,12 +928,9 @@ def send_test_email_for_bulk_emails(tester_id, email_subject, email_body): tester_id: str. The user ID of the tester. email_subject: str. The subject of the email. email_body: str. The body of the email. - - Returns: - None. """ tester_name = user_services.get_username(tester_id) tester_email = user_services.get_email_from_user_id(tester_id) - return _send_email( + _send_email( tester_id, tester_id, feconf.BULK_EMAIL_INTENT_TEST, email_subject, email_body, tester_email, sender_name=tester_name) From 8b354ca11ec09fff74debcf269e9884b7763f64b Mon Sep 17 00:00:00 2001 From: Apurv-Bajaj Date: Thu, 24 May 2018 22:02:39 +0530 Subject: [PATCH 30/36] Fix backend tests --- core/controllers/base.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/core/controllers/base.py b/core/controllers/base.py index da3431a6855b..9fdf431d92e4 100755 --- a/core/controllers/base.py +++ b/core/controllers/base.py @@ -211,6 +211,9 @@ def dispatch(self): Raises: Exception: The CSRF token is missing. UnauthorizedUserException: The CSRF token is invalid. + + Returns: + None. """ # If the request is to the old demo server, redirect it permanently to # the new demo server. @@ -244,7 +247,7 @@ def dispatch(self): '%s: payload %s', e, self.payload) - self.handle_exception(e, self.app.debug) + return self.handle_exception(e, self.app.debug) super(BaseHandler, self).dispatch() From 058b7ee93d38c578acec10c15456be7c0b90fcfe Mon Sep 17 00:00:00 2001 From: Apurv-Bajaj Date: Fri, 25 May 2018 06:36:47 +0530 Subject: [PATCH 31/36] Address all review comments --- .gitignore | 5 - core/controllers/base.py | 6 +- core/domain/stats_jobs_continuous.py | 2 +- scripts/custom_lint_checks.py | 8 +- scripts/custom_lint_checks_test.py | 10 +- .../python-custom-checks/_check_docs_utils.py | 718 ------------------ 6 files changed, 8 insertions(+), 741 deletions(-) delete mode 100644 third_party/python-custom-checks/_check_docs_utils.py diff --git a/.gitignore b/.gitignore index abd564cb8d96..f396a379dc90 100644 --- a/.gitignore +++ b/.gitignore @@ -21,8 +21,3 @@ venv/ # directories for dev and prod. Resource directories for prod are generated # via build.py and should not be committed to the repo. build/* - -# The pre_commit_linter.py script uses _check_docs_utils.py placed -# in python-custom-checks directory under third_party directory. -# Therefore it is not ignored. -!third_party/python-custom-checks diff --git a/core/controllers/base.py b/core/controllers/base.py index 9fdf431d92e4..b5354ca732c0 100755 --- a/core/controllers/base.py +++ b/core/controllers/base.py @@ -211,9 +211,6 @@ def dispatch(self): Raises: Exception: The CSRF token is missing. UnauthorizedUserException: The CSRF token is invalid. - - Returns: - None. """ # If the request is to the old demo server, redirect it permanently to # the new demo server. @@ -247,7 +244,8 @@ def dispatch(self): '%s: payload %s', e, self.payload) - return self.handle_exception(e, self.app.debug) + self.handle_exception(e, self.app.debug) + return super(BaseHandler, self).dispatch() diff --git a/core/domain/stats_jobs_continuous.py b/core/domain/stats_jobs_continuous.py index 6004dc7dca83..3b0e6baa24cd 100644 --- a/core/domain/stats_jobs_continuous.py +++ b/core/domain/stats_jobs_continuous.py @@ -105,7 +105,7 @@ def reduce(key, stringified_values): submitted answers. Yields: - Error. + str. One of the following strings: - Expected a single version when aggregating answers for: Occurs when the versions list contains multiple versions instead of a specific version. diff --git a/scripts/custom_lint_checks.py b/scripts/custom_lint_checks.py index 0f0a9f41bb0e..07afaaf6776a 100644 --- a/scripts/custom_lint_checks.py +++ b/scripts/custom_lint_checks.py @@ -21,6 +21,7 @@ import os import sys +import _check_docs_utils as utils import astroid from pylint.checkers import BaseChecker @@ -28,13 +29,6 @@ from pylint.interfaces import IAstroidChecker from pylint.interfaces import IRawChecker -path = os.path.join('third_party', 'python-custom-checks') -sys.path.insert(0, path) - -# pylint: disable=wrong-import-position -import _check_docs_utils as utils # isort:skip -# pylint: enable=wrong-import-position - class ExplicitKwargsChecker(BaseChecker): """Custom pylint checker which checks for explicit keyword arguments diff --git a/scripts/custom_lint_checks_test.py b/scripts/custom_lint_checks_test.py index 4610385b3759..aaccaee3fd5d 100644 --- a/scripts/custom_lint_checks_test.py +++ b/scripts/custom_lint_checks_test.py @@ -135,20 +135,18 @@ def test_finds_docstring_parameter(self): custom_lint_checks.DocstringParameterChecker) checker_test_object.setup_method() func_node = astroid.extract_node(""" - def test(test_var_one, test_var_two, test_var_three): #@ + def test(test_var_one, test_var_two): #@ \"\"\"Function to test docstring parameters. Args: test_var_one: int. First test variable. test_var_two: str. Second test variable. - test_var_three: list(str). Third test variable. Returns: - str. The test result. - - Yields: - list(str). The test list. + int. The test result. \"\"\" + result = test_var_one + test_var_two + return result """) with checker_test_object.assertNoMessages(): checker_test_object.checker.visit_functiondef(func_node) diff --git a/third_party/python-custom-checks/_check_docs_utils.py b/third_party/python-custom-checks/_check_docs_utils.py deleted file mode 100644 index c74b9eb8cc06..000000000000 --- a/third_party/python-custom-checks/_check_docs_utils.py +++ /dev/null @@ -1,718 +0,0 @@ -# coding: utf-8 - -# The following code was adapted from PyCQA for use -# in the Oppia project. See -# https://github.com/PyCQA/pylint/blob/master/pylint/extensions/_check_docs_utils.py -# for the source origin. -# Licensed under the GPL: https://www.gnu.org/licenses/old-licenses/gpl-2.0.html -# For details: https://github.com/PyCQA/pylint/blob/master/COPYING - -"""Utility methods for docstring checking.""" - -from __future__ import absolute_import -from __future__ import print_function - -import re - -import astroid - -from pylint.checkers import utils - - -def space_indentation(s): - """The number of leading spaces in a string - - Args: - s: str. The input string. - - Returns: - int. The number of leading spaces. - """ - return len(s) - len(s.lstrip(' ')) - - -def get_setters_property_name(node): - """Get the name of the property that the given node is a setter for. - - Args: - node: str. The node to get the property name for. - - Returns: - str|None. The name of the property that the node is a setter for, - or None if one could not be found. - """ - decorators = node.decorators.nodes if node.decorators else [] - for decorator in decorators: - if (isinstance(decorator, astroid.Attribute) and - decorator.attrname == "setter" and - isinstance(decorator.expr, astroid.Name)): - return decorator.expr.name - return None - - -def get_setters_property(node): - """Get the property node for the given setter node. - - Args: - node: astroid.FunctionDef. The node to get the property for. - - Returns: - astroid.FunctionDef|None. The node relating to the property of - the given setter node, or None if one could not be found. - """ - property_ = None - - property_name = get_setters_property_name(node) - class_node = utils.node_frame_class(node) - if property_name and class_node: - class_attrs = class_node.getattr(node.name) - for attr in class_attrs: - if utils.decorated_with_property(attr): - property_ = attr - break - - return property_ - - -def returns_something(return_node): - """Check if a return node returns a value other than None. - - Args: - return_node: astroid.Return. The return node to check. - - Returns: - bool. True if the return node returns a value - other than None, False otherwise. - """ - returns = return_node.value - - if returns is None: - return False - - return not (isinstance(returns, astroid.Const) and returns.value is None) - - -def possible_exc_types(node): - """Gets all of the possible raised exception types for the given raise node. - Caught exception types are ignored. - - - Args: - node: astroid.node_classes.NodeNG. The raise - to find exception types for. - - Returns: - set(str). A list of exception types. - """ - excs = [] - if isinstance(node.exc, astroid.Name): - inferred = utils.safe_infer(node.exc) - if inferred: - excs = [inferred.name] - elif (isinstance(node.exc, astroid.Call) and - isinstance(node.exc.func, astroid.Name)): - target = utils.safe_infer(node.exc.func) - if isinstance(target, astroid.ClassDef): - excs = [target.name] - elif isinstance(target, astroid.FunctionDef): - for ret in target.nodes_of_class(astroid.Return): - if ret.frame() != target: - # return from inner function - ignore it. - continue - - val = utils.safe_infer(ret.value) - if (val and isinstance(val, ( - astroid.Instance, astroid.ClassDef)) and - utils.inherit_from_std_ex(val)): - excs.append(val.name) - elif node.exc is None: - handler = node.parent - while handler and not isinstance(handler, astroid.ExceptHandler): - handler = handler.parent - - if handler and handler.type: - inferred_excs = astroid.unpack_infer(handler.type) - excs = (exc.name for exc in inferred_excs - if exc is not astroid.Uninferable) - - - try: - return set( - exc for exc in excs if not utils.node_ignores_exception( - node, exc)) - except astroid.InferenceError: - return set() - - -def docstringify(docstring): - for docstring_type in [SphinxDocstring, EpytextDocstring, - GoogleDocstring, NumpyDocstring]: - instance = docstring_type(docstring) - if instance.is_valid(): - return instance - - return Docstring(docstring) - - -class Docstring(object): - re_for_parameters_see = re.compile(r""" - For\s+the\s+(other)?\s*parameters\s*,\s+see - """, re.X | re.S) - - supports_yields = None - """True if the docstring supports a "yield" section. - - False if the docstring uses the returns section to document generators. - """ - - # These methods are designed to be overridden - # pylint: disable=no-self-use - def __init__(self, doc): - doc = doc or "" - self.doc = doc.expandtabs() - - def is_valid(self): - return False - - def exceptions(self): - return set() - - def has_params(self): - return False - - def has_returns(self): - return False - - def has_rtype(self): - return False - - def has_property_returns(self): - return False - - def has_property_type(self): - return False - - def has_yields(self): - return False - - def has_yields_type(self): - return False - - def match_param_docs(self): - return set(), set() - - def params_documented_elsewhere(self): - return self.re_for_parameters_see.search(self.doc) is not None - - -class SphinxDocstring(Docstring): - re_type = r"[\w\.]+" - - re_simple_container_type = r""" - {type} # a container type - [\(\[] [^\n\s]+ [\)\]] # with the contents of the container - """.format(type=re_type) - - re_xref = r""" - (?::\w+:)? # optional tag - `{0}` # what to reference - """.format(re_type) - - re_param_raw = r""" - : # initial colon - (?: # Sphinx keywords - param|parameter| - arg|argument| - key|keyword - ) - \s+ # whitespace - - (?: # optional type declaration - ({type}|{container_type}) - \s+ - )? - - (\w+) # Parameter name - \s* # whitespace - : # final colon - """.format(type=re_type, container_type=re_simple_container_type) - re_param_in_docstring = re.compile(re_param_raw, re.X | re.S) - - re_type_raw = r""" - :type # Sphinx keyword - \s+ # whitespace - ({type}) # Parameter name - \s* # whitespace - : # final colon - """.format(type=re_type) - re_type_in_docstring = re.compile(re_type_raw, re.X | re.S) - - re_property_type_raw = r""" - :type: # Sphinx keyword - \s+ # whitespace - {type} # type declaration - """.format(type=re_type) - re_property_type_in_docstring = re.compile( - re_property_type_raw, re.X | re.S - ) - - re_raise_raw = r""" - : # initial colon - (?: # Sphinx keyword - raises?| - except|exception - ) - \s+ # whitespace - - (?: # type declaration - ({type}) - \s+ - )? - - (\w+) # Parameter name - \s* # whitespace - : # final colon - """.format(type=re_type) - re_raise_in_docstring = re.compile(re_raise_raw, re.X | re.S) - - re_rtype_in_docstring = re.compile(r":rtype:") - - re_returns_in_docstring = re.compile(r":returns?:") - - supports_yields = False - - def is_valid(self): - return bool(self.re_param_in_docstring.search(self.doc) or - self.re_raise_in_docstring.search(self.doc) or - self.re_rtype_in_docstring.search(self.doc) or - self.re_returns_in_docstring.search(self.doc) or - self.re_property_type_in_docstring.search(self.doc)) - - def exceptions(self): - types = set() - - for match in re.finditer(self.re_raise_in_docstring, self.doc): - raise_type = match.group(2) - types.add(raise_type) - - return types - - def has_params(self): - if not self.doc: - return False - - return self.re_param_in_docstring.search(self.doc) is not None - - def has_returns(self): - if not self.doc: - return False - - return bool(self.re_returns_in_docstring.search(self.doc)) - - def has_rtype(self): - if not self.doc: - return False - - return bool(self.re_rtype_in_docstring.search(self.doc)) - - def has_property_returns(self): - if not self.doc: - return False - - # The summary line is the return doc, - # so the first line must not be a known directive. - return not self.doc.lstrip().startswith(':') - - def has_property_type(self): - if not self.doc: - return False - - return bool(self.re_property_type_in_docstring.search(self.doc)) - - def match_param_docs(self): - params_with_doc = set() - params_with_type = set() - - for match in re.finditer(self.re_param_in_docstring, self.doc): - name = match.group(2) - params_with_doc.add(name) - param_type = match.group(1) - if param_type is not None: - params_with_type.add(name) - - params_with_type.update(re.findall(self.re_type_in_docstring, self.doc)) - return params_with_doc, params_with_type - - -class EpytextDocstring(SphinxDocstring): - """Epytext is similar to Sphinx. See the docs: - http://epydoc.sourceforge.net/epytext.html - http://epydoc.sourceforge.net/fields.html#fields - - It's used in PyCharm: - https://www.jetbrains.com/help/pycharm/2016.1/creating-documentation-comments.html#d848203e314 - https://www.jetbrains.com/help/pycharm/2016.1/using-docstrings-to-specify-types.html - """ - re_param_in_docstring = re.compile( - SphinxDocstring.re_param_raw.replace(':', '@', 1), - re.X | re.S) - - re_type_in_docstring = re.compile( - SphinxDocstring.re_type_raw.replace(':', '@', 1), - re.X | re.S) - - re_property_type_in_docstring = re.compile( - SphinxDocstring.re_property_type_raw.replace(':', '@', 1), - re.X | re.S) - - re_raise_in_docstring = re.compile( - SphinxDocstring.re_raise_raw.replace(':', '@', 1), - re.X | re.S) - - re_rtype_in_docstring = re.compile(r""" - @ # initial "at" symbol - (?: # Epytext keyword - rtype|returntype - ) - : # final colon - """, re.X | re.S) - - re_returns_in_docstring = re.compile(r"@returns?:") - - def has_property_returns(self): - if not self.doc: - return False - - # If this is a property docstring, the summary is the return doc. - if self.has_property_type(): - # The summary line is the return doc, - # so the first line must not be a known directive. - return not self.doc.lstrip().startswith('@') - - return False - - -class GoogleDocstring(Docstring): - re_type = SphinxDocstring.re_type - - re_xref = SphinxDocstring.re_xref - - re_container_type = r""" - (?:{type}|{xref}) # a container type - [\(\[] [^\n]+ [\)\]] # with the contents of the container - """.format(type=re_type, xref=re_xref) - - re_multiple_type = r""" - (?:{container_type}|{type}|{xref}) - (?:\s+or\s+(?:{container_type}|{type}|{xref}))* - """.format(type=re_type, xref=re_xref, container_type=re_container_type) - - _re_section_template = r""" - ^([ ]*) {0} \s*: \s*$ # Google parameter header - ( .* ) # section - """ - - re_param_section = re.compile( - _re_section_template.format(r"(?:Args|Arguments|Parameters)"), - re.X | re.S | re.M - ) - - re_keyword_param_section = re.compile( - _re_section_template.format(r"Keyword\s(?:Args|Arguments|Parameters)"), - re.X | re.S | re.M - ) - - re_param_line = re.compile(r""" - \s* \*{{0,2}}(\w+) # identifier potentially with asterisks - \s* ( [:] - \s* - ({type}|\S*) - (?:,\s+optional)? - [.] )? \s* # optional type declaration - \s* (.*) # beginning of optional description - """.format( - type=re_multiple_type, - ), re.X | re.S | re.M) - - re_raise_section = re.compile( - _re_section_template.format(r"Raises"), - re.X | re.S | re.M - ) - - re_raise_line = re.compile(r""" - \s* ({type}) \s* : # identifier - \s* (.*) # beginning of optional description - """.format(type=re_type), re.X | re.S | re.M) - - re_returns_section = re.compile( - _re_section_template.format(r"Returns?"), - re.X | re.S | re.M - ) - - re_returns_line = re.compile(r""" - \s* (({type}|\S*).)? # identifier - \s* (.*) # beginning of description - """.format( - type=re_multiple_type, - ), re.X | re.S | re.M) - - re_property_returns_line = re.compile(r""" - ^{type}: # indentifier - \s* (.*) # Summary line / description - """.format( - type=re_multiple_type, - ), re.X | re.S | re.M) - - re_yields_section = re.compile( - _re_section_template.format(r"Yields?"), - re.X | re.S | re.M - ) - - re_yields_line = re_returns_line - - supports_yields = True - - def is_valid(self): - return bool(self.re_param_section.search(self.doc) or - self.re_raise_section.search(self.doc) or - self.re_returns_section.search(self.doc) or - self.re_yields_section.search(self.doc) or - self.re_property_returns_line.search(self._first_line())) - - def has_params(self): - if not self.doc: - return False - - return self.re_param_section.search(self.doc) is not None - - def has_returns(self): - if not self.doc: - return False - - entries = self._parse_section(self.re_returns_section) - for entry in entries: - match = self.re_returns_line.match(entry) - if not match: - continue - - return_desc = match.group(2) - if return_desc: - return True - - return False - - def has_rtype(self): - if not self.doc: - return False - - entries = self._parse_section(self.re_returns_section) - for entry in entries: - match = self.re_returns_line.match(entry) - if not match: - continue - - return_type = match.group(1) - if return_type: - return True - - return False - - def has_property_returns(self): - # The summary line is the return doc, - # so the first line must not be a known directive. - first_line = self._first_line() - return not bool(self.re_param_section.search(first_line) or - self.re_raise_section.search(first_line) or - self.re_returns_section.search(first_line) or - self.re_yields_section.search(first_line)) - - def has_property_type(self): - if not self.doc: - return False - - return bool(self.re_property_returns_line.match(self._first_line())) - - def has_yields(self): - if not self.doc: - return False - - entries = self._parse_section(self.re_yields_section) - for entry in entries: - match = self.re_yields_line.match(entry) - if not match: - continue - - yield_desc = match.group(2) - if yield_desc: - return True - - return False - - def has_yields_type(self): - if not self.doc: - return False - - entries = self._parse_section(self.re_yields_section) - for entry in entries: - match = self.re_yields_line.match(entry) - if not match: - continue - - yield_type = match.group(1) - if yield_type: - return True - - return False - - def exceptions(self): - types = set() - - entries = self._parse_section(self.re_raise_section) - for entry in entries: - match = self.re_raise_line.match(entry) - if not match: - continue - - exc_type = match.group(1) - exc_desc = match.group(2) - if exc_desc: - types.add(exc_type) - - return types - - def match_param_docs(self): - params_with_doc = set() - params_with_type = set() - - entries = self._parse_section(self.re_param_section) - entries.extend(self._parse_section(self.re_keyword_param_section)) - for entry in entries: - match = self.re_param_line.match(entry) - if not match: - continue - - param_name = match.group(1) - param_type = match.group(2) - param_desc = match.group(3) - if param_type: - params_with_type.add(param_name) - - if param_desc: - params_with_doc.add(param_name) - - return params_with_doc, params_with_type - - def _first_line(self): - return self.doc.lstrip().split('\n', 1)[0] - - @staticmethod - def min_section_indent(section_match): - return len(section_match.group(1)) + 1 - - @staticmethod - def _is_section_header(_): - # Google parsing does not need to detect section headers, - # because it works off of indentation level only. - return False - - def _parse_section(self, section_re): - section_match = section_re.search(self.doc) - if section_match is None: - return [] - - min_indentation = self.min_section_indent(section_match) - - entries = [] - entry = [] - is_first = True - for line in section_match.group(2).splitlines(): - if not line.strip(): - continue - indentation = space_indentation(line) - if indentation < min_indentation: - break - - # The first line after the header defines the minimum - # indentation. - if is_first: - min_indentation = indentation - is_first = False - - if indentation == min_indentation: - if self._is_section_header(line): - break - # Lines with minimum indentation must contain the beginning - # of a new parameter documentation. - if entry: - entries.append("\n".join(entry)) - entry = [] - - entry.append(line) - - if entry: - entries.append("\n".join(entry)) - - return entries - - -class NumpyDocstring(GoogleDocstring): - _re_section_template = r""" - ^([ ]*) {0} \s*?$ # Numpy parameters header - \s* [-=]+ \s*?$ # underline - ( .* ) # section - """ - - re_param_section = re.compile( - _re_section_template.format(r"(?:Args|Arguments|Parameters)"), - re.X | re.S | re.M - ) - - re_param_line = re.compile(r""" - \s* (\w+) # identifier - \s* : - \s* (?:({type})(?:,\s+optional)?)? # optional type declaration - \n # description starts on a new line - \s* (.*) # description - """.format( - type=GoogleDocstring.re_multiple_type, - ), re.X | re.S) - - re_raise_section = re.compile( - _re_section_template.format(r"Raises"), - re.X | re.S | re.M - ) - - re_raise_line = re.compile(r""" - \s* ({type})$ # type declaration - \s* (.*) # optional description - """.format(type=GoogleDocstring.re_type), re.X | re.S | re.M) - - re_returns_section = re.compile( - _re_section_template.format(r"Returns?"), - re.X | re.S | re.M - ) - - re_returns_line = re.compile(r""" - \s* (?:\w+\s+:\s+)? # optional name - ({type})$ # type declaration - \s* (.*) # optional description - """.format( - type=GoogleDocstring.re_multiple_type, - ), re.X | re.S | re.M) - - re_yields_section = re.compile( - _re_section_template.format(r"Yields?"), - re.X | re.S | re.M - ) - - re_yields_line = re_returns_line - - supports_yields = True - - @staticmethod - def min_section_indent(section_match): - return len(section_match.group(1)) - - @staticmethod - def _is_section_header(line): - return bool(re.match(r'\s*-+$', line)) From 34968605addd2e50a36cf845d6838a93ceb961a4 Mon Sep 17 00:00:00 2001 From: Apurv-Bajaj Date: Fri, 25 May 2018 06:37:11 +0530 Subject: [PATCH 32/36] Add doc utils --- scripts/_check_docs_utils.py | 188 +++++++++++++++++++++++++++++++++++ 1 file changed, 188 insertions(+) create mode 100644 scripts/_check_docs_utils.py diff --git a/scripts/_check_docs_utils.py b/scripts/_check_docs_utils.py new file mode 100644 index 000000000000..8595c31daa36 --- /dev/null +++ b/scripts/_check_docs_utils.py @@ -0,0 +1,188 @@ +# coding: utf-8 +# +# Copyright 2018 The Oppia Authors. All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS-IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +from __future__ import absolute_import +from __future__ import print_function + +import os +import re +import sys + +_PARENT_DIR = os.path.abspath(os.path.join(os.getcwd(), os.pardir)) +_PYLINT_PATH = os.path.join(_PARENT_DIR, 'oppia_tools', 'pylint-1.8.4') +sys.path.insert(0, _PYLINT_PATH) + +# pylint: disable=wrong-import-position +import astroid # isort:skip +from pylint.checkers import utils # isort:skip +from pylint.extensions import _check_docs_utils # isort:skip +# pylint: enable=wrong-import-position + + +def space_indentation(s): + """The number of leading spaces in a string + + Args: + s: str. The input string. + + Returns: + int. The number of leading spaces. + """ + return len(s) - len(s.lstrip(' ')) + + +def get_setters_property_name(node): + """Get the name of the property that the given node is a setter for. + + Args: + node: str. The node to get the property name for. + + Returns: + str|None. The name of the property that the node is a setter for, + or None if one could not be found. + """ + decorators = node.decorators.nodes if node.decorators else [] + for decorator in decorators: + if (isinstance(decorator, astroid.Attribute) and + decorator.attrname == "setter" and + isinstance(decorator.expr, astroid.Name)): + return decorator.expr.name + return None + + +def get_setters_property(node): + """Get the property node for the given setter node. + + Args: + node: astroid.FunctionDef. The node to get the property for. + + Returns: + astroid.FunctionDef|None. The node relating to the property of + the given setter node, or None if one could not be found. + """ + property_ = None + + property_name = get_setters_property_name(node) + class_node = utils.node_frame_class(node) + if property_name and class_node: + class_attrs = class_node.getattr(node.name) + for attr in class_attrs: + if utils.decorated_with_property(attr): + property_ = attr + break + + return property_ + + +def returns_something(return_node): + """Check if a return node returns a value other than None. + + Args: + return_node: astroid.Return. The return node to check. + + Returns: + bool. True if the return node returns a value + other than None, False otherwise. + """ + returns = return_node.value + + if returns is None: + return False + + return not (isinstance(returns, astroid.Const) and returns.value is None) + + +def possible_exc_types(node): + """Gets all of the possible raised exception types for the given raise node. + Caught exception types are ignored. + + Args: + node: astroid.node_classes.NodeNG. The raise + to find exception types for. + + Returns: + set(str). A list of exception types. + """ + excs = [] + if isinstance(node.exc, astroid.Name): + inferred = utils.safe_infer(node.exc) + if inferred: + excs = [inferred.name] + elif (isinstance(node.exc, astroid.Call) and + isinstance(node.exc.func, astroid.Name)): + target = utils.safe_infer(node.exc.func) + if isinstance(target, astroid.ClassDef): + excs = [target.name] + elif isinstance(target, astroid.FunctionDef): + for ret in target.nodes_of_class(astroid.Return): + if ret.frame() != target: + # return from inner function - ignore it. + continue + + val = utils.safe_infer(ret.value) + if (val and isinstance(val, ( + astroid.Instance, astroid.ClassDef)) and + utils.inherit_from_std_ex(val)): + excs.append(val.name) + elif node.exc is None: + handler = node.parent + while handler and not isinstance(handler, astroid.ExceptHandler): + handler = handler.parent + + if handler and handler.type: + inferred_excs = astroid.unpack_infer(handler.type) + excs = (exc.name for exc in inferred_excs + if exc is not astroid.Uninferable) + + + try: + return set( + exc for exc in excs if not utils.node_ignores_exception( + node, exc)) + except astroid.InferenceError: + return set() + + +def docstringify(docstring): + for docstring_type in [GoogleDocstring]: + instance = docstring_type(docstring) + if instance.is_valid(): + return instance + + return _check_docs_utils.Docstring(docstring) + + +class GoogleDocstring(_check_docs_utils.GoogleDocstring): + + re_multiple_type = _check_docs_utils.GoogleDocstring.re_multiple_type + re_param_line = re.compile(r""" + \s* \*{{0,2}}(\w+) # identifier potentially with asterisks + \s* ( [:] + \s* + ({type}|\S*) + (?:,\s+optional)? + [.] )? \s* # optional type declaration + \s* (.*) # beginning of optional description + """.format( + type=re_multiple_type, + ), re.X | re.S | re.M) + + re_returns_line = re.compile(r""" + \s* (({type}|\S*).)? # identifier + \s* (.*) # beginning of description + """.format( + type=re_multiple_type, + ), re.X | re.S | re.M) From dae48e7d9f3ba442ae1f4d93afe173c017eedcf6 Mon Sep 17 00:00:00 2001 From: Apurv-Bajaj Date: Fri, 25 May 2018 06:43:16 +0530 Subject: [PATCH 33/36] Add yields line --- scripts/_check_docs_utils.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/scripts/_check_docs_utils.py b/scripts/_check_docs_utils.py index 8595c31daa36..ddafc374257e 100644 --- a/scripts/_check_docs_utils.py +++ b/scripts/_check_docs_utils.py @@ -186,3 +186,5 @@ class GoogleDocstring(_check_docs_utils.GoogleDocstring): """.format( type=re_multiple_type, ), re.X | re.S | re.M) + + re_yields_line = re_returns_line From 3cc5d335587c964e9d6653f264906bb4b22edb35 Mon Sep 17 00:00:00 2001 From: Apurv-Bajaj Date: Fri, 25 May 2018 06:55:09 +0530 Subject: [PATCH 34/36] lint fix --- scripts/custom_lint_checks.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/scripts/custom_lint_checks.py b/scripts/custom_lint_checks.py index 07afaaf6776a..cc9070e2f219 100644 --- a/scripts/custom_lint_checks.py +++ b/scripts/custom_lint_checks.py @@ -18,9 +18,6 @@ from __future__ import division from __future__ import print_function -import os -import sys - import _check_docs_utils as utils import astroid From f398be43798f7ea09fb61762408b9e06a8b2f933 Mon Sep 17 00:00:00 2001 From: Apurv-Bajaj Date: Fri, 25 May 2018 09:51:32 +0530 Subject: [PATCH 35/36] Review changes --- core/domain/stats_jobs_continuous.py | 2 +- core/domain/user_services.py | 4 ++-- core/platform/email/gae_email_services.py | 3 +-- core/storage/classifier/gae_models.py | 2 +- scripts/custom_lint_checks.py | 20 +++++++++---------- ...ck_docs_utils.py => docstrings_checker.py} | 11 +++++----- scripts/install_third_party.py | 2 +- scripts/pre_commit_linter.py | 8 +------- 8 files changed, 22 insertions(+), 30 deletions(-) rename scripts/{_check_docs_utils.py => docstrings_checker.py} (96%) diff --git a/core/domain/stats_jobs_continuous.py b/core/domain/stats_jobs_continuous.py index 3b0e6baa24cd..c286e6c80cd3 100644 --- a/core/domain/stats_jobs_continuous.py +++ b/core/domain/stats_jobs_continuous.py @@ -113,7 +113,7 @@ def reduce(key, stringified_values): Occurs when there is not exactly one interaction ID for each exploration and version. - Expected at least one item ID for exploration: - Occurs when there is not atleast one Item ID for + Occurs when there is not at least one Item ID for each exploration and version. - Ignoring answers submitted to version: Occurs when version mismatches and the new diff --git a/core/domain/user_services.py b/core/domain/user_services.py index 72f216721de2..44d6e886f224 100644 --- a/core/domain/user_services.py +++ b/core/domain/user_services.py @@ -110,9 +110,9 @@ def __init__( user last edited an exploration. profile_picture_data_url: str or None. User uploaded profile picture as a dataURI string. - default_dashboard: str|None. The dashboard the user wants. + default_dashboard: str|None. The default dashboard of the user. creator_dashboard_display_pref: str. The creator dashboard - preference the user wants. + dashboard of the user. user_bio: str. User-specified biography. subject_interests: list(str) or None. Subject interests specified by the user. diff --git a/core/platform/email/gae_email_services.py b/core/platform/email/gae_email_services.py index 0c7920fab6dc..8508f39957fd 100644 --- a/core/platform/email/gae_email_services.py +++ b/core/platform/email/gae_email_services.py @@ -92,8 +92,7 @@ def send_bulk_mail( Args: sender_email: str. The email address of the sender. This should be in the form 'SENDER_NAME '. - recipient_emails: list(str). The list of email addresses - of the recipients. + recipient_emails: list(str). The list of recipients' email addresses. subject: str. The subject line of the email. plaintext_body: str. The plaintext body of the email. html_body: str. The HTML body of the email. Must fit in a datastore diff --git a/core/storage/classifier/gae_models.py b/core/storage/classifier/gae_models.py index 38e7ee919ab9..1b90506f60b2 100644 --- a/core/storage/classifier/gae_models.py +++ b/core/storage/classifier/gae_models.py @@ -285,7 +285,7 @@ def create( @classmethod def create_multi(cls, job_exploration_mappings): - """Creates multiple new TrainingJobExplorationMappingModel entries. + """Creates multiple new TrainingJobExplorationMappingModel entries. Args: job_exploration_mappings: list(TrainingJobExplorationMapping). The diff --git a/scripts/custom_lint_checks.py b/scripts/custom_lint_checks.py index cc9070e2f219..a9ac6d2ad3b3 100644 --- a/scripts/custom_lint_checks.py +++ b/scripts/custom_lint_checks.py @@ -18,8 +18,8 @@ from __future__ import division from __future__ import print_function -import _check_docs_utils as utils import astroid +import docstrings_checker from pylint.checkers import BaseChecker from pylint.checkers import utils as checker_utils @@ -263,7 +263,7 @@ def visit_functiondef(self, node): node: astroid.scoped_nodes.Function. Node for a function or method definition in the AST. """ - node_doc = utils.docstringify(node.doc) + node_doc = docstrings_checker.docstringify(node.doc) self.check_functiondef_params(node, node_doc) self.check_functiondef_returns(node, node_doc) self.check_functiondef_yields(node, node_doc) @@ -273,7 +273,7 @@ def check_functiondef_params(self, node, node_doc): if node.name in self.constructor_names: class_node = checker_utils.node_frame_class(node) if class_node is not None: - class_doc = utils.docstringify(class_node.doc) + class_doc = docstrings_checker.docstringify(class_node.doc) self.check_single_constructor_params( class_doc, node_doc, class_node) @@ -304,7 +304,7 @@ def check_functiondef_returns(self, node, node_doc): if (( node_doc.has_returns() or node_doc.has_rtype()) and not any( - utils.returns_something( + docstrings_checker.returns_something( ret_node) for ret_node in return_nodes)): self.add_message( 'redundant-returns-doc', @@ -325,18 +325,18 @@ def visit_raise(self, node): if not isinstance(func_node, astroid.FunctionDef): return - expected_excs = utils.possible_exc_types(node) + expected_excs = docstrings_checker.possible_exc_types(node) if not expected_excs: return if not func_node.doc: # If this is a property setter, # the property should have the docstring instead. - property_ = utils.get_setters_property(func_node) + property_ = docstrings_checker.get_setters_property(func_node) if property_: func_node = property_ - doc = utils.docstringify(func_node.doc) + doc = docstrings_checker.docstringify(func_node.doc) if not doc.is_valid(): if doc.doc: self._handle_no_raise_doc(expected_excs, func_node) @@ -347,14 +347,14 @@ def visit_raise(self, node): self._add_raise_message(missing_excs, func_node) def visit_return(self, node): - if not utils.returns_something(node): + if not docstrings_checker.returns_something(node): return func_node = node.frame() if not isinstance(func_node, astroid.FunctionDef): return - doc = utils.docstringify(func_node.doc) + doc = docstrings_checker.docstringify(func_node.doc) if not doc.is_valid() and self.config.accept_no_return_doc: return @@ -379,7 +379,7 @@ def visit_yield(self, node): if not isinstance(func_node, astroid.FunctionDef): return - doc = utils.docstringify(func_node.doc) + doc = docstrings_checker.docstringify(func_node.doc) if not doc.is_valid() and self.config.accept_no_yields_doc: return diff --git a/scripts/_check_docs_utils.py b/scripts/docstrings_checker.py similarity index 96% rename from scripts/_check_docs_utils.py rename to scripts/docstrings_checker.py index ddafc374257e..09f2f445b22d 100644 --- a/scripts/_check_docs_utils.py +++ b/scripts/docstrings_checker.py @@ -129,7 +129,6 @@ def possible_exc_types(node): elif isinstance(target, astroid.FunctionDef): for ret in target.nodes_of_class(astroid.Return): if ret.frame() != target: - # return from inner function - ignore it. continue val = utils.safe_infer(ret.value) @@ -168,22 +167,22 @@ def docstringify(docstring): class GoogleDocstring(_check_docs_utils.GoogleDocstring): re_multiple_type = _check_docs_utils.GoogleDocstring.re_multiple_type - re_param_line = re.compile(r""" + re_param_line = re.compile(r''' \s* \*{{0,2}}(\w+) # identifier potentially with asterisks \s* ( [:] \s* ({type}|\S*) (?:,\s+optional)? - [.] )? \s* # optional type declaration + [.] )? \s* # optional type declaration \s* (.*) # beginning of optional description - """.format( + '''.format( type=re_multiple_type, ), re.X | re.S | re.M) - re_returns_line = re.compile(r""" + re_returns_line = re.compile(r''' \s* (({type}|\S*).)? # identifier \s* (.*) # beginning of description - """.format( + '''.format( type=re_multiple_type, ), re.X | re.S | re.M) diff --git a/scripts/install_third_party.py b/scripts/install_third_party.py index 3c8dbbfdfa85..763e4e06dae3 100644 --- a/scripts/install_third_party.py +++ b/scripts/install_third_party.py @@ -201,7 +201,7 @@ def test_manifest_syntax(dependency_type, dependency_dict): Display warning message when there is an error and terminate the program. Args: - dependency_type: dependency_download_format. + dependency_type: str. Dependency download format. dependency_dict: dict. manifest.json dependency dict. """ keys = dependency_dict.keys() diff --git a/scripts/pre_commit_linter.py b/scripts/pre_commit_linter.py index 63e99df963b9..5247566b0ddf 100644 --- a/scripts/pre_commit_linter.py +++ b/scripts/pre_commit_linter.py @@ -821,14 +821,8 @@ def _check_docstrings(all_files): summary_messages = [] files_to_check = [ filename for filename in all_files if not - (any( - fnmatch.fnmatch(filename, pattern) for pattern in EXCLUDED_PATHS)) - and not (filename.endswith('_check_docs_utils.py')) + any(fnmatch.fnmatch(filename, pattern) for pattern in EXCLUDED_PATHS) and filename.endswith('.py')] - # The file _check_docs_utils.py has to be excluded from this check - # as it contains a number of regular expressions which trigger this - # check. The only way to do this was to exclude this from - # files_to_check. missing_period_message = ( 'There should be a period at the end of the docstring.') multiline_docstring_message = ( From 5769c5ffa029e3ba921b5462e2a11f874e0a009a Mon Sep 17 00:00:00 2001 From: Apurv-Bajaj Date: Fri, 25 May 2018 10:48:22 +0530 Subject: [PATCH 36/36] Review changes --- scripts/custom_lint_checks.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/custom_lint_checks.py b/scripts/custom_lint_checks.py index a9ac6d2ad3b3..fbfe328d6cf5 100644 --- a/scripts/custom_lint_checks.py +++ b/scripts/custom_lint_checks.py @@ -149,8 +149,8 @@ def process_module(self, node): 'no-break-after-hanging-indent', line=line_num + 1) -# This class has been derived from -# https://github.com/PyCQA/pylint/blob/master/pylint/extensions/docparams.py#L26. +# The following class was derived from +# https://github.com/PyCQA/pylint/blob/377cc42f9e3116ff97cddd4567d53e9a3e24ebf9/pylint/extensions/docparams.py#L26 class DocstringParameterChecker(BaseChecker): """Checker for Sphinx, Google, or Numpy style docstrings